-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Add join table reference check in JOIN ON clause #17460
Add join table reference check in JOIN ON clause #17460
Conversation
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.
Nice!
String warningMessage = tableName.isPresent() | ||
? |
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.
put ?
to the end of last line
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.
done
tableName.get())) | ||
: |
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.
put :
to the end of the last line
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.
done
rightTableName = ((Table) rightRelation).getName().toString(); | ||
else if (relation instanceof AliasedRelation) { | ||
AliasedRelation aliasedRelation = (AliasedRelation) relation; | ||
if (aliasedRelation.getRelation() instanceof Table) { |
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 call tryGetTable here on the aliasedRelation.getRelation()?
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.
I do not see tryGetTable()
method anywhere, only tryGetTableName()
. And you are pointing to the body of tryGetTableName()
.
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.
yeah tryGetTableName, it could have kept the code cleaner. Anyways @highker merged the PR.
Often times, while specifying the joining condition in JOIN ON clause, users make mistakes and not referencing the joining table along side with other table in join condition that result in performance issue due to conditional join resulting in CROSS JOIN. This change will extend the number of cases when user is shown the PERFORMANCE_WARNING when JOIN ON clause misses the comparison expression with joining table and other table. Specifically, it covers the cases when user does not explicitly refer to the relation through table name or alias in JOIN ON clause. Resolves:prestodb#17382 See also: prestodb#17333
b08b8dc
to
97db2f2
Compare
Often times, while specifying the joining condition in JOIN ON clause,
users make mistakes and not referencing the joining table along side
with other table in join condition that result in performance issue due
to conditional join resulting in CROSS JOIN.
This change will extend the number of cases when user is shown the
PERFORMANCE_WARNING when JOIN ON clause misses the comparison expression
with joining table and other table. Specifically, it covers the cases
when user does not explicitly refer to the relation through table name
or alias in JOIN ON clause.
Resolves:#17382
See also: #17333