-
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
refactor: Run QTT using the testing tool #3180
refactor: Run QTT using the testing tool #3180
Conversation
Merge remote-tracking branch 'upstream/master' into QTT-using-testing-tool
] | ||
}, | ||
{ | ||
"name": "import session stream and group by", |
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.
This test is incorrect since at the moment kstreams does not support reading windowed key and aggregating on it!
{"topic": "S2", "key": "0 : Window{start=0 end=10000}", "value": "0 : Window{start=0 end=10000},1", "timestamp": 10000}, | ||
{"topic": "S2", "key": "1 : Window{start=10000 end=10000}", "value": "1 : Window{start=10000 end=10000},1", "timestamp": 10000}, | ||
{"topic": "S2", "key": "1 : Window{start=10000 end=10000}", "value": "1 : Window{start=10000 end=10000},2", "timestamp": 10000} | ||
{"topic": "S2", "key": 0, "value": "0,0", "timestamp": 0, "window": {"start": 0, "end": 9223372036854775807, "type": "time"}}, |
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 end of the window should be Long.MAX_VALUE since in legacy mode we read this as a time window with no size.
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 have quite a few inline comments trying to understand your changes, but the "core" of the change (what swaps us to actually use testing tool instead of the old path) LGTM.
...tional-tests/src/main/java/io/confluent/ksql/test/serde/avro/ValueSpecAvroSerdeSupplier.java
Outdated
Show resolved
Hide resolved
...tional-tests/src/main/java/io/confluent/ksql/test/serde/avro/ValueSpecAvroSerdeSupplier.java
Show resolved
Hide resolved
...tional-tests/src/main/java/io/confluent/ksql/test/serde/avro/ValueSpecAvroSerdeSupplier.java
Outdated
Show resolved
Hide resolved
...tional-tests/src/main/java/io/confluent/ksql/test/serde/avro/ValueSpecAvroSerdeSupplier.java
Outdated
Show resolved
Hide resolved
ksql-functional-tests/src/main/java/io/confluent/ksql/test/model/TopicNode.java
Outdated
Show resolved
Hide resolved
ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestCase.java
Outdated
Show resolved
Hide resolved
ksql-functional-tests/src/main/java/io/confluent/ksql/test/tools/TestCase.java
Show resolved
Hide resolved
ksql-functional-tests/src/test/java/io/confluent/ksql/test/EndToEndEngineTestUtil.java
Outdated
Show resolved
Hide resolved
ksql-functional-tests/src/test/java/io/confluent/ksql/test/EndToEndEngineTestUtil.java
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.
LGTM! Some nits inline
This PR changes the QTT execution to use the testing tool. QTTs will be a special case for the testing tool where we only run one query!
The PR handles
KSQL_WINDOWED_SESSION_KEY_LEGACY_CONFIG
, however, we should remove it after resolving #3179 .I also fixed a bug with the union types in the QTT where we did not distinguish between unions of two and more than two elements.
Also I fixed a bug in QTT where the map entries were not converted correctly into avro format.
Testing:
All QTT tests run correctly with the new flow.
Reviewer checklist