Open
Conversation
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
jgao54
force-pushed
the
handle-empty-tuple
branch
from
eb124bf to
dd0c3aa
Compare
jgao54
force-pushed
the
handle-empty-tuple
branch
from
dd0c3aa to
b81f1a5
Compare
ilidemi
reviewed
Feb 24, 2026
| slices.ContainsFunc(tableMapping.Columns, func(col *protos.ColumnSetting) bool { | ||
| return col.Ordering > 0 | ||
| }) | ||
| if !hasOrderingKeys && tableMapping.Engine != protos.TableEngine_CH_ENGINE_NULL { |
Contributor
There was a problem hiding this comment.
&& tableMapping.Engine != protos.TableEngine_CH_ENGINE_MERGE_TREE
(if we're following the breaking change)
| var settingVal string | ||
| if err := QueryRow(ctx, logger, conn, | ||
| "SELECT value FROM system.settings WHERE name = 'allow_suspicious_primary_key'", | ||
| ).Scan(&settingVal); err == nil && settingVal == "1" { |
Contributor
There was a problem hiding this comment.
A network error here would result in a confusing message
| } | ||
|
|
||
| return fmt.Errorf( | ||
| "source table %s has no primary key columns; add a primary key to the source table, "+ |
Contributor
There was a problem hiding this comment.
This function doesn't really know about primary keys on the source, it only checks a setting? It also implicitly depends on the caller validating the dest sorting keys and table engine
Comment on lines
+83
to
+86
| hasOrderingKeys := len(processedSchema.PrimaryKeyColumns) > 0 || | ||
| slices.ContainsFunc(tableMapping.Columns, func(col *protos.ColumnSetting) bool { | ||
| return col.Ordering > 0 | ||
| }) |
Contributor
There was a problem hiding this comment.
What would the behavior be for missing primary key, missing sorting keys but CDC on and replica identity full? As I understand it, setup flow would just take all columns as src primary key + dest sorting key in this case, and this change would validate away that scenario (but I might be wrong). Not sure if there's a way to unify all that logic somehow, possible there's not.
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
2 participants
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.
In ClickHouse 25.12,
ORDER BY TUPLE()is no longer supported. This has never worked properly in the past because snapshot would end up collapsing into a single row when PK is not specified in source database. So this PR will throw a validation error when order by empty tuple is detected.The scenarios where empty tuple is allowed:
allow_suspicious_primary_keyis enabled