-
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
feat: Introduce materialization support for tables using CTAS to make tables queryable #7085
Conversation
@confluentinc It looks like @cprasad1 just signed our Contributor License Agreement. 👍 Always at your service, clabot |
ksqldb-engine/src/main/java/io/confluent/ksql/engine/PullQueryExecutionUtil.java
Outdated
Show resolved
Hide resolved
...s/src/test/resources/rest-query-validation-tests/pull-queries-against-ctas-materialized.json
Outdated
Show resolved
Hide resolved
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.
Thanks, @cprasad1 !
I can't speak to many of the little changes in internal method calls, etc., but at a high level, the error message and the tests look good: exactly what we wanted to achieve.
Minor note: we prefer "queryable" to "queriable".
Also, specifically in the tests, I'm wondering if we can re-structure them to be a little more compact and have better coverage, something like this:
- Have each case set up a CT or CTAS statement for query in a slightly different way, covering all the possible permutations (like a basic select *, one that includes a WHERE clause, one that includes a more complicated projection, one that does both, one that does a groupBy agg from a stream, one that does a join, etc. etc.).
- Then, inside each case, we just run multiple pull queries, for each slight variant of pull queries we want to test
In other words, we vary the queriable object between tests and vary the pill queries within each test. That way, we strike a good balance between code coverage and proliferation of test cases.
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 just had a few small comments, but looks good.
...gine/src/test/java/io/confluent/ksql/materialization/ks/KsMaterializationFunctionalTest.java
Outdated
Show resolved
Hide resolved
ksqldb-execution/src/main/java/io/confluent/ksql/execution/plan/KTableHolder.java
Outdated
Show resolved
Hide resolved
ksqldb-streams/src/main/java/io/confluent/ksql/execution/streams/TableTableJoinBuilder.java
Outdated
Show resolved
Hide resolved
...s/src/test/resources/rest-query-validation-tests/pull-queries-against-ctas-materialized.json
Outdated
Show resolved
Hide resolved
ksqldb-engine/src/main/java/io/confluent/ksql/physical/pull/PullPhysicalPlanBuilder.java
Outdated
Show resolved
Hide resolved
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.
The PR LGTM, but I had a meta question regarding the description:
CREATE TABLE QUERYABLE_TABLE AS SELECT...
Are we really going to augment the syntax? In this PR it seems we just make CTAT always materialized anyways.
ksqldb-execution/src/main/java/io/confluent/ksql/execution/plan/KTableHolder.java
Outdated
Show resolved
Hide resolved
ksqldb-streams/src/main/java/io/confluent/ksql/execution/streams/TableTableJoinBuilder.java
Outdated
Show resolved
Hide resolved
@guozhangwang what are you referring to when you say "augment"? If you were referring to the |
My bad, I thought it was some new keywords :P |
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.
Thanks, @cprasad1 !
Description
Introduces materialization support for tables using CTAS to make tables queryable.
We can make tables queryable by using:
CREATE TABLE QUERYABLE_TABLE AS SELECT...
This would help fix #4614 by adding a more helpful error message as well.
Testing done
Reviewer checklist