-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
support for WindowSegment #26038
support for WindowSegment #26038
Conversation
@strongduanmu Please review, is the approach good ? |
888f23a
to
81a0d39
Compare
81a0d39
to
a3ce5c5
Compare
Hi @kanha-gupta |
14aea7d
to
8b36f17
Compare
@strongduanmu @RaigorJiang Please review. |
...java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/MySQLStatementVisitor.java
Outdated
Show resolved
Hide resolved
return new IdentifierValue(ctx.colId().identifier().getText()); | ||
} | ||
|
||
private Collection<ExpressionSegment> getWindowSpecification(final WindowSpecificationContext ctx) { |
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.
Hi @kanha-gupta, can you add sql parse test case for new visit logic?
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.
sir, I have added sql parse test case in the new xml file for Mysql, PostgreSQL & opengauss.
is there any other test to add ?
8b36f17
to
9a125c7
Compare
@strongduanmu Please review again, changes are made :) |
} | ||
|
||
private Collection<ExpressionSegment> getWindowSpecification(final WindowSpecificationContext ctx) { | ||
Collection<ExpressionSegment> result = createInsertValuesSegments(ctx.partitionClause().exprList()); |
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.
Hi @kanha-gupta, why call createInsertValuesSegments method in getWindowSpecification?
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.
Sir, thank you for the review :) createInsertValuesSegments method Gives desired Result for exprList() processing therefore I call that method instead of writing code for it.
Is the approach good or Should I write new code for it ?
Thank you
...main/java/org/apache/shardingsphere/sql/parser/sql/common/segment/generic/WindowSegment.java
Outdated
Show resolved
Hide resolved
Will this issue be completed before June 15 which is the final date of version 5.4.0? |
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.
@kanha-gupta Thank you for your outstanding work.
Thanks a lot sir :) |
* support for WindowSegment * corrections * refactoring window segment
Ref #24200
support for caseID:
1.select_group_by_with_having_and_window
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e
.