You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/04/08 08:04:00 UTC
[GitHub] [incubator-doris] Henry2SS opened a new pull request, #8919: [Feature] (sql digest) support sql digest
Henry2SS opened a new pull request, #8919:
URL: https://github.com/apache/incubator-doris/pull/8919
# Proposed changes
Issue Number: close #8918
## Problem Summary:
Describe the overview of changes.
## Checklist(Required)
1. Does it affect the original behavior: (No)
2. Has unit tests been added: (Yes)
3. Has document been added or modified: (No Need)
4. Does it need to update dependencies: (No)
5. Are there any changes that cannot be rolled back: (No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] github-actions[bot] commented on pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#issuecomment-1120223733
PR approved by at least one committer and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Henry2SS commented on a diff in pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
Henry2SS commented on code in PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#discussion_r847072683
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java:
##########
@@ -871,12 +871,20 @@ 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();
+ protected String toDigestImpl() {
+ return toSqlImpl();
Review Comment:
Thx for your reply.
The abstract method `toSqlImpl()` will be overrided in each subclass. So there is no need to throw out exceptions.
The only situation is that a newly added subclass, which its sql digest needs to be different from sql impl, should override this method.
Added comments over this method to remind developers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman merged pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
morningman merged PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Henry2SS commented on pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
Henry2SS commented on PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#issuecomment-1120182488
> please fix conflicts ❤️
Done~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Henry2SS commented on a diff in pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
Henry2SS commented on code in PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#discussion_r850234649
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AnalyticExpr.java:
##########
@@ -831,6 +831,36 @@ public String toSqlImpl() {
return sb.toString();
}
+ @Override
+ public String toDigestImpl() {
+ StringBuilder sb = new StringBuilder();
+ sb.append(fnCall.toDigest()).append(" OVER (");
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morrySnow commented on pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
morrySnow commented on PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#issuecomment-1120369759
please fix fe code style error, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morrySnow commented on pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
morrySnow commented on PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#issuecomment-1113527085
please fix conflicts ❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a diff in pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
morningman commented on code in PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#discussion_r849388957
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AnalyticExpr.java:
##########
@@ -831,6 +831,36 @@ public String toSqlImpl() {
return sb.toString();
}
+ @Override
+ public String toDigestImpl() {
+ StringBuilder sb = new StringBuilder();
+ sb.append(fnCall.toDigest()).append(" OVER (");
Review Comment:
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
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AnalyticExpr.java:
##########
@@ -831,6 +831,36 @@ public String toSqlImpl() {
return sb.toString();
}
+ @Override
+ public String toDigestImpl() {
+ StringBuilder sb = new StringBuilder();
+ sb.append(fnCall.toDigest()).append(" OVER (");
Review Comment:
I think we don't need those `OVER` or `PARTITION BY` keywords in digest?
maybe just a prefix like `AnalyticExpr` is enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a diff in pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
morningman commented on code in PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#discussion_r846254884
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java:
##########
@@ -871,12 +871,20 @@ 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();
+ protected String toDigestImpl() {
+ return toSqlImpl();
Review Comment:
Maybe we should throw `NotImplementedException` here?
So that we can ensure that every newly added Expr subclass will override this method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] Henry2SS commented on pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
Henry2SS commented on PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#issuecomment-1105108075
> Also need to update audit loader plugin:
>
> ```
> fe_plugins/auditloader/src/main/java/org/apache/doris/plugin/audit/AuditLoaderPlugin.java
> fe_plugins/auditloader/src/main/java/org/apache/doris/plugin/audit/DorisStreamLoader.java
> ```
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] github-actions[bot] commented on pull request #8919: [Feature] (sql digest) support sql digest
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8919:
URL: https://github.com/apache/incubator-doris/pull/8919#issuecomment-1120223739
PR approved by anyone and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Feature] (sql digest) support sql digest [doris]
Posted by "2416210017 (via GitHub)" <gi...@apache.org>.
2416210017 commented on PR #8919:
URL: https://github.com/apache/doris/pull/8919#issuecomment-3223592426
@Henry2SS @morrySnow @morningman Does Doris 2.1.5 have a configuration parameter to enable the recording of all SQL sql-digests in the audit log?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org