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

Fix issues with bigint sum -> numeric conversion #5299

Merged
merged 2 commits into from
Jan 14, 2021

Conversation

ruchirK
Copy link
Contributor

@ruchirK ruchirK commented Jan 13, 2021

Functional changes here in the first commit:

  • make sum_int64 have the same behavior as the rest of the reduce path
  • make our reduce elision logic output the correct type

and then theres a small debug output change to pgwire in the second commit.

I tested this manually against Chris's query and it works but I'm not sure if thats suitable for an integration test or if we should do something else. Afaik we're not able to specify primary keys from sql primitives. @benesch do you have any thoughts?


This change is Reviewable

Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but you think we could wrap up the bug into a test case somehow?

@ruchirK
Copy link
Contributor Author

ruchirK commented Jan 13, 2021

Code looks good, but you think we could wrap up the bug into a test case somehow?

Yeah I'm not sure:

We could definitely repeat the query on the mz_scheduling_elapsed log. I'm ok with doing that in a testdrive test + a comment indicating why. Doing this does seem very brittle though.

Other than that:

  • I don't know how to declare primary keys on sources, views or tables. Oh wait I actually just thought of something as I was typing -- views with reductions have primary keys. so i think attempting to double compute the sum of a view will hit this elision bug. I can do that.

  • I don't know how to force sum_int64 to execute

@benesch
Copy link
Member

benesch commented Jan 13, 2021

In SLT you can just create a table with a primary key actually!

@ruchirK
Copy link
Contributor Author

ruchirK commented Jan 13, 2021

tests added!

Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Looks great to me.

Copy link
Contributor

@cirego cirego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this!

@ruchirK
Copy link
Contributor Author

ruchirK commented Jan 13, 2021 via email

@ruchirK ruchirK merged commit f2634ad into MaterializeInc:main Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants