-
-
Notifications
You must be signed in to change notification settings - Fork 422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CTE not working #124
Comments
q version 1.5.0 |
Does sqlite3 depend on q having support for CTE? If so, maybe q was made prior to CTE's in sqlite3. |
Hi, sorry for the late reply, i'm on a long vacation... Most sqlite capabilities are exposed through q without any special needs. However, CTE is not one of them. I'll take a deeper look and see if it's possible to add it. Thanks |
Thanks for getting back on this! The query works in sqlite3, so if you decide not to add CTEs (at least in the near term), you could just change this sentence on the site: And add stuff to the "Limitations" section. |
Of course, i'll definitely add this to the limitations if I can't come up with something good. |
this is a pretty big missing functionality. something like this makes it easier to generate histograms, for instance. I've been using q regularly, such a perfect use-case. But now I want to do more, and my workflow is pushing toward copying file to local postgres db. If q can't run CTE, and I have to migrate workflow to postgres local db, there is not much sense in continuing to use |
I don't really know about CTEs but it seems their usecase which can not be achieved with subqueries is in recursive queries. If you don't need recursive queries you should be fine with using subqueries, which are not supported by q in the from clause but there is a workaround: #102 |
yes you are right that a subquery satisfies CTE functionality, and I don't need recursive CTE for generic use-cases in my work. I guess the note about sqlite getting CTE functionality gave me hope. I'm aware of the subquery limitation and the workaround, but based on your reminder I took another look at #102. It works great, although I have to lookup the quoting syntax pretty much every time. On balance, this is easier than the steps of creating a schema in postgres, and copying the file in. Still -- if its possible to gain the new sqlite CTE functionality, it would be excellent. |
Can |
Yes, AS is supported, and actually needed when performing JOINs |
|
Maybe special table names like diff --git a/bin/q.py b/bin/q.py
index b41451f..de773bf 100755
--- a/bin/q.py
+++ b/bin/q.py
@@ -406,6 +406,9 @@ class Sql(object):
qtable_name = qtable_name[:qtable_name.index(')')]
self.sql_parts[idx + 1] = qtable_name
+ if qtable_name.startswith("nofile_"):
+ idx += 1
+ continue
self.qtable_names.add(qtable_name)
if qtable_name not in self.qtable_name_positions.keys(): Or maybe there can be implemented a mode where each file-to-table mapping is provided separately and individually? |
@vi The issue is not with reading literal SELECTs (e.g. Perhaps there's a new package for analyzing the SQL in python which can help with this. I'll do some checking. |
With the diff above some my use cases which required saving temporary CSV files start working without temporary files. |
@vi the idea of q is to provide seamless use of SQL, without any need for special casing with other parameters (e.g. user knowledge). If the CTE use case will get more upvotes, i'd consider it a higher priority for trying to find a proper solution for this. |
Modifying the suggestion from @vi so it checks to see if qtable_name refers to a file seems to work and avoids the need for special table names. @harelba do you see any issues with this approach.
|
@EdMUK Will this lead to less user-friendly error messages in case of misspelled filenames? |
Hi, I don't think that checking for an actual file would be the best approach. However, I'm looking into the "filename escaping" solution that was suggested by @vi a month ago. Something along these lines could prove very useful to solve this issue without too much risk or new development. This would render q's SQL to be less compatible with standard sqlite3, but together with some command line flag to enable it, it could be a reasonable solution. Another solution i'm looking into is to detect the "WITH X" parts and provide automatic "escaping" behind the scenes, which would be a more transparent solution. I'll update. Best of health to everyone during these corona times. |
I would like to test any solution before actually releasing a version, since the parsing logic is delicate as it is (I always wanted to use a proper AST parser for this, which would have made solving this trivial, but never got to it because the existing solution worked well enough for most cases). Anyway, I'll create a branch when done, and ask you politely to download it manually and do some tests before going forward with this. Hope that won't be too much of a hassle for you. Harel |
Pushed an initial PR (#223 ) with a fix, but can't really test it on my computer for dumb reasons. Wanted travis to perform the testing, but it seems to have an issue as well with bringing up the osx containers. I'll update when it's successful. You're welcome to comment on the fix (and find any holes in it...). Harel |
@vi @mahiki @EdMUK @bitti @anthezium Would any of you mind manually doing some testing on the branch of PR #223 before i merge it? I don't have much experience with crazy CTE queries, so want to try and make sure that the fix is good enough and doesn't break anything. These are complicated times, and I'm sure that all of you have a lot on your mind, so obviously that totally fine if you can't spend time on this. |
My solution is too simplistic there, it will work only for simple use cases. I'll work towards the filename "escaping" solution, and update. |
Is the filename escaping implement in some form? |
This should work, right? (emails.csv is there, works with other queries)
q -d, -H "WITH alias AS (SELECT * FROM ./emails.csv) SELECT * FROM alias"
Instead, I get:
No files matching 'alias' have been found
Do I need to do something special to let it know that the alias is an alias?
The text was updated successfully, but these errors were encountered: