-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(tesseract): Initial BigQuery support #9577
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9577 +/- ##
=======================================
Coverage 84.10% 84.10%
=======================================
Files 230 230
Lines 84409 84409
=======================================
Hits 70995 70995
Misses 13414 13414
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a few minor questions and suggestions.
templates.operators.is_not_distinct_from = 'IS NOT DISTINCT FROM'; | ||
templates.join_types.full = 'FULL'; | ||
templates.statements.time_series_select = 'SELECT DATETIME(TIMESTAMP(f)) date_from, DATETIME(TIMESTAMP(t)) date_to \n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be much nicer to define it in backtick multiline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a debatable suggestion. Manual line concatenation helps keep both the Jinja logic and the rendered result readable.
'{% for time_item in seria %}' +
' select \'{{ time_item[0] }}\' f, \'{{ time_item[1] }}\' t \n' +
'{% if not loop.last %} UNION ALL\n{% endif %}' +
'{% endfor %}' +
Converting it to a backtick multiline would cause an extra newline to be inserted before every select in the output.
And if I were to collapse the for and select into the same line, the template would become much harder to read — and the rendered SQL would also lose formatting by placing all SELECTs on one line.
Check List