Skip to content
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

Bigquery: GROUP BY error #249

Open
mgkahn opened this issue Apr 18, 2021 · 9 comments
Open

Bigquery: GROUP BY error #249

mgkahn opened this issue Apr 18, 2021 · 9 comments
Labels
bug Erroneous behavior that must be fixed help wanted Will probably not be addressed by the package maintainer, but could be addressed by someone else

Comments

@mgkahn
Copy link

mgkahn commented Apr 18, 2021

Two screenshots from SqlRender Developer showing GROUP BY arguments mapped incorrectly for BigQuery. Second screenshot shows me replacing column names with column numbers and the mapping is still incorrect. Postgres mapping was correct for both constructs. As best as my limited understanding allows, I do not think this is an error in the rewrite patterns but somewhere deep in the BG-specific java parsing code, well beyond me. If anybody has a suggestion how to rewrite the original (first screenshot/top panel) to process correctly thru TRANSLATE, please let me know.
Thanks, Michael

image

image

@konstjar
Copy link

@mgkahn Thank you for asking. Let us check and to propose solution.

@ssuvorov-fls
Copy link
Contributor

SqlRender currently does not process correctly numbers less or equal to number of arguments in select statement.
max(case when ingredient_start_date=ingredient_start_date_new then 10 else 0 end) will be translated correctly

Or you can move case statement into inner select. Something like this:

select
	person_id,
	ingredient_start_date_new,
	max(original_ingredient)
into
	#regimens
from
	(
	select
		person_id,
		ingredient_start_date_new,
		case
			when ingredient_start_date = ingredient_start_date_new then 1
			else 0
		end as original_ingredient
	from
		#add_groups g) t
group by
	ingredient_start_date_new,
	person_id
order by
	ingredient_start_date_new;

@mgkahn
Copy link
Author

mgkahn commented Apr 19, 2021

I am confused by this answer. The issue is the GROUP BY arguments not the CASE statement. SQLRender translate the GROUP BY as 2,3 rather than 2,1 like is done correctly for Postgres.

Are you saying the CASE statement throws off the GROUP BY translation?

Thanks for looking into this more.
Michael

@ssuvorov-fls
Copy link
Contributor

No, it's bug in SqlRender.
There're several iterations during translation.
1 iter - SqlRender searches for column names from group by clause in select clause and substitutes their position in select clause instead of their names in group by clause (so we get group by 2, 1 and your sql statement from first screenshot begins to look like the statement from the second screenshot)
2 iter - SqlRender performs this search again, finds 1 in your aggregate function and substitute its position (3) instead of 1. And your group by clause becomes group by 2, 3

@mgkahn
Copy link
Author

mgkahn commented Apr 19, 2021

Thank you for the further insights. Not to be annoying but one last question (maybe!): why does the translation work correctly for Postgres but not BG. If I understand your explanation, the mis-processing should occur in both translations since it seems to be a DBMS independent part of the code. Why BigQuery specific?

@ssuvorov-fls
Copy link
Contributor

BigQuery uses its own translation mechanism due to differences of their dialects

@vojtechhuser
Copy link

vojtechhuser commented Jul 12, 2021

This is a problem for us as well. Because AllOfUs uses bigquery and we use AoU.

see replacement on a simple example

Why can't group by be kept as is and is changed to numbers? What replacement pattern (what row in that pattern file) is doing that change? (or what other process besides the pattern file)

image

@vojtechhuser
Copy link

here is the actual query that crashes for us because of the rendering

image

@schuemie schuemie added bug Erroneous behavior that must be fixed help wanted Will probably not be addressed by the package maintainer, but could be addressed by someone else labels Jun 29, 2022
@ssuvorov-fls
Copy link
Contributor

@ schumie
it seems to be already fixed
could you check it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Erroneous behavior that must be fixed help wanted Will probably not be addressed by the package maintainer, but could be addressed by someone else
Projects
None yet
Development

No branches or pull requests

5 participants