-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Feature] (sql digest) support sql digest #8919
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -874,12 +874,25 @@ public String toSql() { | |
| return (printSqlInParens) ? "(" + toSqlImpl() + ")" : toSqlImpl(); | ||
| } | ||
|
|
||
| public String toDigest() { | ||
| return (printSqlInParens) ? "(" + toDigestImpl() + ")" : toDigestImpl(); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a SQL string representing this expr. Subclasses should override this method | ||
| * instead of toSql() to ensure that parenthesis are properly added around the toSql(). | ||
| */ | ||
| protected abstract String toSqlImpl(); | ||
|
|
||
| /** | ||
| * !!!!!! Important !!!!!! | ||
| * Subclasses should override this method if | ||
| * sql digest should be represented different from tosqlImpl(). | ||
| */ | ||
| protected String toDigestImpl() { | ||
| return toSqlImpl(); | ||
|
Contributor
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. Maybe we should throw
Contributor
Author
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. Thx for your reply. Added comments over this method to remind developers. |
||
| } | ||
|
|
||
| public String toMySql() { | ||
| return toSql(); | ||
| } | ||
|
|
@@ -952,6 +965,14 @@ public List<String> childrenToSql() { | |
| // } | ||
| // } | ||
|
|
||
| public List<String> childrenToDigest() { | ||
| List<String> childrenDigestList = Lists.newArrayList(); | ||
| for (Expr child : children) { | ||
| childrenDigestList.add(child.toDigest()); | ||
| } | ||
| return childrenDigestList; | ||
| } | ||
|
|
||
| public static com.google.common.base.Predicate<Expr> isAggregatePredicate() { | ||
| return IS_AGGREGATE_PREDICATE; | ||
| } | ||
|
|
||
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 think we don't need those
OVERorPARTITION BYkeywords in digest?maybe just a prefix like
AnalyticExpris enough.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.
And I'm not quite sure about the performance impact of this implementation.
Looks like these are too much string operations.
Have you test its performance? Especially in high concurrency senario.
And I found some reference about how to implement digest or fingerprint of a SQL:
https://segmentfault.com/a/1190000040132966
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.
Thx.
I've tested locally, there is a 10% query performance loss. It looks bad.
So decide to put it in
auditAfterExec()and only for slow queries.