-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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(schema-compiler) Resolve rollup join pre-aggregation param allocator issue #8279
Open
pauldheinrichs
wants to merge
8
commits into
cube-js:master
Choose a base branch
from
pauldheinrichs:ph/8268
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
1406add
fix(schema-compiler) Resolve rollup join pre-aggregation param alloca…
pauldheinrichs 660e7f2
re add default return
pauldheinrichs d4ae4a9
clean up comments and initalization
pauldheinrichs bc895df
Remove these logging
pauldheinrichs a009a0e
unset here too!
pauldheinrichs 52aa3c1
had it backwards
pauldheinrichs 9f3c5ce
Revert "had it backwards"
pauldheinrichs 57c31b2
Revert "unset here too!"
pauldheinrichs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2651,13 +2651,18 @@ export class BaseQuery { | |
|
||
newSubQuery(options) { | ||
const QueryClass = this.constructor; | ||
return new QueryClass(this.compilers, this.subQueryOptions(options)); | ||
return new QueryClass(this.compilers, { ...this.subQueryOptions(options), paramAllocator: undefined }); | ||
} | ||
|
||
newSubQueryForCube(cube, options) { | ||
return this.options.queryFactory | ||
? this.options.queryFactory.createQuery(cube, this.compilers, this.subQueryOptions(options)) | ||
: this.newSubQuery(options); | ||
if (this.options.queryFactory) { | ||
// When dealing with rollup joins, it's crucial to use the correct parameter allocator for the specific cube in use. | ||
// By default, we'll use BaseQuery, but it's important to note that different databases (Oracle, PostgreSQL, MySQL, Druid, etc.) | ||
// have unique parameter allocator symbols. Using the wrong allocator can break the query, especially when rollup joins involve | ||
// different cubes that require different allocators. | ||
return this.options.queryFactory.createQuery(cube, this.compilers, { ...this.subQueryOptions(options), paramAllocator: undefined }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unset the paramAllocator set by |
||
} | ||
return this.newSubQuery(options); | ||
} | ||
|
||
subQueryOptions(options) { | ||
|
Oops, something went wrong.
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.
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.
We can't unset param allocator here as it should reused in case of actual
sub_query: true
members.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.
What would you recommend here then, reverting the last commit and pushing forward with just the initial implementation?