Skip to content
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

Column bind update in with segment bind #34384

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Yash-cor
Copy link
Contributor

@Yash-cor Yash-cor commented Jan 17, 2025

Fixes #ISSUSE_ID.

Changes proposed in this pull request:

While using With Clause when Column definition is added it caused Column not found exception.
Added DifferenceInColumnCountOfSelectListAndColumnNameListException to handle difference in Column counts.
Made correction in column bind and externalTableBinderContext in CommonTableExpressionBinder.


Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.
  • I have updated the Release Notes of the current development version. For more details, see Update Release Note

@strongduanmu
Copy link
Member

Hi @Yash-cor, can you update release note?

@Yash-cor
Copy link
Contributor Author

Hi @Yash-cor, can you update release note?

Yes Sir

@Yash-cor
Copy link
Contributor Author

Hi @Yash-cor, can you update release note?

Hello @strongduanmu I have updated the release notes.

return new SimpleTableSegmentBinderContext(commonTableExpressionSegment.getSubquery().getSelect().getProjections().getProjections());
} else {
Collection<ProjectionSegment> projectionSegments = new LinkedList<>();
commonTableExpressionSegment.getColumns().forEach(each -> projectionSegments.add(new ColumnProjectionSegment(each)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this logic to CommonTableExpressionSegmentBinder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @strongduanmu while changing the logic I found Error in the way WithSegment is handling Recursive segment of with clause.

  • Recursive segment allows user to use the same common table expression ( cte ) name in cte's subquery
    Example -

     WITH RECURSIVE cte  AS
    (
       SELECT 1 as (n)
       UNION ALL
       SELECT n + 1 FROM cte WHERE n < 5
    )
    SELECT * FROM cte;
    

    or similarly can be written as

    WITH RECURSIVE cte (n) AS
    (
       SELECT 1
       UNION ALL
       SELECT n + 1 FROM cte WHERE n < 5
    )
    SELECT * FROM cte;
    
  • The WITH clause must begin with WITH RECURSIVE if any CTE in the WITH clause refers to itself. (If no CTE refers to itself, RECURSIVE is permitted but not required.) We have to include a check for it.

  • The recursive CTE subquery has two parts, separated by UNION ALL or UNION DISTINCT. Thus, a recursive CTE consists of a non-recursive SELECT part followed by a recursive SELECT part. Should i include a check for it as this check happens when the query hits the MySQL database.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this logic to CommonTableExpressionSegmentBinder?

I have added this logic in CommonTableExpressionSegmentBinder inside the recursion condition.

</from>
</select>
</subquery-expression>
<column name="col1" start-index="18" stop-index="21"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add column-bound expected result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the column-bound info

@Yash-cor Yash-cor requested a review from strongduanmu January 22, 2025 10:19
@@ -66,4 +67,14 @@ public static CommonTableExpressionSegment bind(final CommonTableExpressionSegme
result.getColumns().addAll(segment.getColumns());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check it again, these columns need call ColumnSegmentBinder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes help in ColumnSegmentBind

<original-column name="remark" start-delimiter="`" end-delimiter="`" />
</column-bound>
</column>
<column name="col6" start-index="48" stop-index="51">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check SQLStatementAssert to confirm whether these columns have been asserted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I have checked these asserts.

Copy link
Contributor Author

@Yash-cor Yash-cor Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @strongduanmu could you give any updates on this PR.

@Yash-cor Yash-cor marked this pull request as draft January 24, 2025 06:43
@Yash-cor Yash-cor marked this pull request as ready for review January 24, 2025 09:58
@Yash-cor Yash-cor requested a review from strongduanmu January 24, 2025 09:58
@Yash-cor Yash-cor marked this pull request as draft January 27, 2025 06:12
@Yash-cor Yash-cor marked this pull request as ready for review January 27, 2025 06:12
@Yash-cor
Copy link
Contributor Author

Yash-cor commented Feb 7, 2025

@strongduanmu I think the label should be bug as it causes ColumnNotFoundException when we define column names while using with segment.

@strongduanmu
Copy link
Member

@strongduanmu I think the label should be bug as it causes ColumnNotFoundException when we define column names while using with segment.

Obviously, the SQL Bind logic of the WITH statement is still under development, and setting the label to enhancement is more in line with the actual situation.

binderContext.getExternalTableBinderContexts().put(new CaseInsensitiveString(result.getAliasName().get()),
createWithTableBinderContext(result, binderContext));
}
result.getColumns().clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove result.getColumns().addAll(segment.getColumns()); and result.getColumns().clear();, they seem meaningless.

Copy link
Contributor Author

@Yash-cor Yash-cor Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reply @strongduanmu
This segment result.getColumns().addAll(segment.getColumns()); and result.getColumns().clear(); are needed for creating ExternalTableBinderContexts.
ExternalTableBinderContexts need the result with Column Segment and binded SubqueryTableSegment.
And in order to perform column bind for CommonTableExpressionSegment we need the updated ExternalTableBinderContexts.

@Yash-cor Yash-cor requested a review from strongduanmu February 7, 2025 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants