-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore: primitive keys for simple queries #4096
chore: primitive keys for simple queries #4096
Conversation
First of a few commits to start introducing support for primitive keys in different query types. This commit opens the door for CT/CS statements with primitive keys, (`STRING`, `INT`, `BIGINT`, `BOOLEAN` and `DOUBLE`), and for using those sources in non-join, non-aggregate and non-partition-by queries.
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.
LGTM
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.
I think there are copy paste errors in the exception messages.
@@ -93,6 +94,10 @@ public JoinNode( | |||
: KeyField.of(leftKeyCol.ref()); | |||
|
|||
this.schema = JoinParamsFactory.createSchema(left.getSchema(), right.getSchema()); | |||
|
|||
if (schema.key().get(0).type().baseType() != SqlBaseType.STRING) { | |||
throw new KsqlException("GROUP BY is not supported with non-STRING keys"); |
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.
Should this be "JOIN is not supported"?
@@ -45,6 +47,10 @@ public RepartitionNode( | |||
this.source = Objects.requireNonNull(source, "source"); | |||
this.partitionBy = Objects.requireNonNull(partitionBy, "partitionBy"); | |||
this.keyField = Objects.requireNonNull(keyField, "keyField"); | |||
|
|||
if (source.getSchema().key().get(0).type().baseType() != SqlBaseType.STRING) { | |||
throw new KsqlException("GROUP BY is not supported with non-STRING keys"); |
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.
Should this be "PARTITION BY is not supported"?
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.
Oh I see, you fixed the error messages in the stacked #4098
LGTM then
Description
fixes: #3534
First of a few commits to start introducing support for primitive keys in different query types.
This commit opens the door for CT/CS statements with primitive keys, (
STRING
,INT
,BIGINT
,BOOLEAN
andDOUBLE
), and for using those sources in non-join, non-aggregate and non-partition-by queries.Testing done
Example queries added to
key-schemas.json
, which is a good place to start reviewing.Reviewer checklist