Some BigQuery tweaks [ci skip] #23367
Conversation
| (reconcile-temporal-types | ||
| ((get-method sql.qp/->honeysql [:sql filter-type]) | ||
| driver | ||
| (reconcile-temporal-types clause))))) |
There was a problem hiding this comment.
Interesting, reconciling twice.
There was a problem hiding this comment.
Doesn't really hurt anything
There was a problem hiding this comment.
In which cases is it useful? (I'm asking because I suppose you have cases in mind I haven't though about.)
There was a problem hiding this comment.
It wouldn't work if we didn't reconcile after compilation. Not sure if the initial reconciliation before compilation is still needed or not. I didn't check. You can try taking it out and seeing if it works or not
There was a problem hiding this comment.
Intuitively, I would think it's an abuse of reconciliation if we have to do it twice. I might be wrong though.
| (if (or (not add-fn) | ||
| (and (not (contains? (temporal-type->supported-units t) unit)) | ||
| (contains? (temporal-type->supported-units :datetime) unit))) | ||
| (recur (->temporal-type :datetime hsql-form)) |
There was a problem hiding this comment.
Can you give an example when recur happens more than once in this loop?
There was a problem hiding this comment.
No... it shouldn't happen more than once
There was a problem hiding this comment.
If you're pointing out that we don't need to use loop here since it's not a loop at all then I'm not disagreeing with you. Feel free to change it. I just took the existing code and tweaked it a bit and didn't do cleanup like that.
There was a problem hiding this comment.
Are 1. and 2. the first and the second iteration? Wouldn't we set :datetime after 1. and see it in 2.?
What input for the function do you have in mind?
| (defn x [] | ||
| (mt/with-driver :bigquery-cloud-sdk | ||
| (mt/dataset office-checkins | ||
| (mt/with-temp-vals-in-db 'Field (mt/id :checkins :timestamp) {:database_type "TIMESTAMP"} |
There was a problem hiding this comment.
For some annoying reason office-checkins.checkins.timestamp is actually a DATETIME for BigQuery so I have to fake making it a TIMESTAMP here.
| (AddIntervalForm. hsql-form amount unit))) | ||
| (add-interval-form hsql-form amount unit))) | ||
|
|
||
| (defn x [] |
There was a problem hiding this comment.
test function for seeing the HoneySQL form we generate in the failing case
| [:relative-datetime -2 :year] | ||
| [:relative-datetime -1 :year]])))))) | ||
|
|
||
| (defn y [] |
There was a problem hiding this comment.
test function to see the compiled SQL
| [:field (mt/id :checkins :timestamp) nil] | ||
| [:interval 7 :day]] | ||
| [:relative-datetime -2 :year] | ||
| [:relative-datetime -1 :year]])))))) |
There was a problem hiding this comment.
Is there some good trick to trace the transformation steps? (I guess even that wouldn't tell me at what point things go wrong, but it might give some overview.)
There was a problem hiding this comment.
(metabase.test/set-ns-log-level! :trace) will set the log level for the current namespace to TRACE and then any log/trace statements in there will get printed out. That's what I usually do. You can do that in metabase.driver.bigquery-cloud-sdk.query-processor and maybe metabase.driver.sql.query-processor as well. Add more log/trace statements if you want to see more stuff
This is just so I can show @metamben what I'm talking about. It's just a demo it can be closed later.
Discussion is in #23292