[DAPHNE-#494] SQLParser: Group by without aggregation clause not working #543
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The shape inference for GroupOp did indeed have a bug, but this was not the right fix (using inferNumColsFromArgs() was generally wrong). - Simplified the new test case for GROUP BY without aggregation.
pdamme
approved these changes
Jun 17, 2023
There was a problem hiding this comment.
Thanks @tomschw, this contribution fixes the bug. I polished it just a bit.
There was indeed also a bug in the shape inference of GroupOp, but the problem there was not to check if the list of aggregation columns is empty. The actual problem was the use of inferNumColsFromArgs(), a shape inference helper function not suitable for this case. Instead, one should just take the size() of the list of key columns and aggregation columns. I adjusted it accordingly.
pdamme
added a commit
that referenced
this pull request
Apr 8, 2024
…teToCallKernelOpPass. - Some DaphneIR operations have variadic operands, i.e., operands which can occur an arbitrary number of times (including zero). - When lowering DaphneIR operations to the DaphneIR CallKernelOp, this poses a problem as the C++ kernels cannot be pre-compiled for any number of operands. - Thus, the kernels expect an array of operands (pointer plus size); in DaphneIR this is represented by a value of type daphne::VariadicPack, which stores all occurrences of a variadic operand. - So RewriteToCallKernelOpPass needs to convert variadic operands of DaphneIR operations to DaphneIR's VariadicPack. - In the past there have been problems in case of variadic operands with zero occurrences. - As GroupOp was the only problematic op so far, a workaround has been provided in PRs #564 and #543. - This commit improves the treatment of variadic operands with zero occurrences in RewriteToCallKernelOpPass: - First, the code is more general now and clearly expresses that this problem is not about GroupOp, but about instances of ops with variadic operands with zero occurrences, in general. - Second, the code for treating individual such ops is significantly simpler and more extensible now, since one only has to provide a default MLIR type for the variadic operand. - Furthermore, a simplified version of the example DaphneDSL code mentioned in issue #562, that failed before PR #564, was added as a test case now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed problem:
Changes:
Other possible solutions instead of the current workaround:
As the Kernel lowering should be reworked in the future anyway, the current workaround should work fine for the moment.
Fixes #494.