Open
Conversation
Contributor
Author
|
@claude[agent] review this PR. |
Code review completed. This is a high-quality PR with comprehensive JOIN support, multi-column ARRAY JOIN, USING syntax, and proper FINAL/SAMPLE fixes. Strengths:
Minor suggestions:
Recommendation: APPROVE - Code is production-ready, suggestions are non-blocking improvements for developer experience. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces and fixes a number of core SQLAlchemy core issues.
Comprehensive
JOINsupport: Newch_join()helper andClickHouseJoinclass now supports all ClickHouse strictness modifiers (ALL,ANY,SEMI,ANTI,ASOF),GLOBALdistribution modifier, and explicitCROSS JOIN. Use withselect_from()to generate ClickHouse-specific join syntax likeGLOBAL ALL LEFT OUTER JOIN.Closes SQLAlchemy: JOIN modifiers (ALL/ANY/ASOF, GLOBAL) #635
Multi-column
ARRAY JOIN:array_join()now accepts a list of columns with optional per-column aliases for parallel array expansion. GeneratesARRAY JOIN col1 AS a, col2 AS b, col3 AS c. Single-column usage is unchanged and backward compatible, including direct ArrayJoin constructor calls.Closes SQLAlchemy: ARRAY JOIN with multiple columns #633
JOIN USINGsupport:ch_join()now supportsusing=["col1", "col2"]to generateUSING (col1, col2)instead ofON. This is needed forFULL OUTER JOINwhereUSINGmerges the join column correctly whileONproduces default values(0, '')for unmatched sides. This also includes proper cache key differentiation.Closes SQLAlchemy: add support for JOIN USING syntax #636
FINAL/SAMPLEfix: Fixed.final()and.sample()silently overwriting each other when chained on the same select.Closes SQLAlchemy: .final() and .sample() silently overwrite each other when chained #658
VALUEScompiler fix: Fixedvisit_valuesto use ClickHouse'sVALUES(structure, tuples)table function syntax instead of standard SQL rendering.Closes SQLAlchemy: VALUES function is constructed incorrectly #681
SQAlchemt versions
It's worth noting that SQLAlchemy 1.4 support is still required for Superset integration. However, a small number of features require SQLAlchemy 2.x. E.g. Values.cte() and certain literal-rendering behaviors, but otherwise both 1.4 and 2.x are still considered supported.
Checklist
Delete items not relevant to your PR: