fix(clickhouse): unwrap parenthesized PRIMARY_KEY independent of ORDER_BY#5881
Open
anxkhn wants to merge 1 commit into
Open
fix(clickhouse): unwrap parenthesized PRIMARY_KEY independent of ORDER_BY#5881anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
…R_BY The PRIMARY_KEY branch in _build_table_properties_exp tested the ORDER_BY value (ordered_by_raw) for exp.Paren instead of the primary_key value, a copy-paste leftover from the ORDER_BY block above it. As a result a single parenthesized PRIMARY_KEY such as PRIMARY_KEY = (a) was only unwrapped when ORDER_BY also happened to be a Paren; with a bare column, a tuple, or no ORDER_BY the parentheses were kept and the adapter emitted the malformed PRIMARY KEY ((a)). The generated primary key wrongly depended on the unrelated ORDER_BY shape. Check primary_key for exp.Paren so it is unwrapped regardless of ORDER_BY, and add regression cases pairing PRIMARY_KEY = (a) with a bare and a tuple ORDER_BY. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
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.
The ClickHouse engine adapter builds the
PRIMARY KEYclause in_build_table_properties_exp. When thePRIMARY_KEYphysical property is asingle parenthesized column (for example
PRIMARY_KEY = (a)), sqlglot parses itinto an
exp.Parenthat has to be unwrapped, otherwise the adapter emits adoubly-parenthesized
PRIMARY KEY ((a)).The unwrap guard checked the wrong value: it tested
ordered_by_raw(theORDER_BYvalue) forexp.Pareninstead ofprimary_key. This looks like acopy-paste leftover from the
ORDER_BYblock just above it, which correctly usesordered_by_raw. As a result the parenthesizedPRIMARY_KEYwas only unwrappedwhen
ORDER_BYhappened to be aParentoo. With a bare column, a tuple, or noORDER_BYat all, the parentheses were kept and the generated primary key cameout malformed:
So the emitted primary key wrongly depended on the unrelated
ORDER_BYshape.The fix checks
primary_keyforexp.Paren, making thePRIMARY_KEYblocksymmetric with the
ORDER_BYblock above it. It is a one-line change to theguard.
Test Plan
Added two regression assertions to
tests/core/engine_adapter/test_clickhouse.py::test_model_properties, pairingPRIMARY_KEY = (a)with a bareORDER_BY = aand with a tupleORDER_BY = (a, b); both now assertPRIMARY KEY (a). The existing case in thattest used
ORDER_BY = (a)(also aParen), which is exactly why the bug wasmasked. Reverting the source line makes the new bare-
ORDER_BYassertion failwith
PRIMARY KEY ((a)); restoring it passes.I did not mark the full
make fast-testchecklist item because the broad suiteis currently blocked in this local environment by an unrelated
pyodbc/ systemunixODBCimport failure intests/core/test_connection_config.py::test_mssql_pyodbc_connection_string_generation.The ClickHouse-specific regression coverage above passes.
Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO