Skip to content
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

fix: support WindowStart() and WindowEnd() in pull queries #4435

Conversation

big-andy-coates
Copy link
Contributor

Description

fixes: #4015

At present WindowStart() and WindowEnd() UDAFs are special cased. This special casing was not being applied to pull queries. This change corrects this.

Consider:

-- setup:
CREATE STREAM ALL_TWEETS (LANG STRING) WITH (kafka_topic='test_topic', value_format='JSON');
CREATE TABLE TWEET_LANG AS SELECT TIMESTAMPTOSTRING(WINDOWSTART(),'yyyy-MM-dd HH:mm:ss Z', 'UTC') AS WSTART, TIMESTAMPTOSTRING(WINDOWEND(),'yyyy-MM-dd HH:mm:ss Z', 'UTC') AS WEND, LANG, COUNT(*) AS TWEET_COUNT FROM ALL_TWEETS WINDOW TUMBLING (SIZE 1 SECOND) GROUP BY LANG;

-- pull query:
SELECT WSTART, WEND, LANG, TWEET_COUNT FROM TWEET_LANG WHERE ROWKEY='en';

Before this change the pull query would return null values for WSTART and WEND, even though the data was correctly populated in the TWEET_LANG topic. With this change the correct values for WSTART and WEND are returned.

How to review

The main fix is in StreamAggregateBuilder, which now applies a suitable map call that will apply the special processing to any pull query.

Adding the map call highlighted that the wrong key was being passed to the transformers provided in the map call when processing windowed keys: it was passing the raw key, i.e. Struct, not the Windowed<Struct>key. Fixing this required:

  • Changing WindowedRow to take the Windowed<Struct> key.
  • Changing KsMaterializedSessionTable to use the windowed key to build the windowed row.
  • Changing KsMaterializedWindowTable to construct a TimeWindow and use this to build the windowed row.

The rest are test changes.

Testing done

Suitable test added to QTT.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

fixes: confluentinc#4015

At present `WindowStart()` and `WindowEnd()` UDAFs are special cased.  This special casing was not being applied to pull queries. This change corrects this.

Consider:

```json
-- setup:
CREATE STREAM ALL_TWEETS (LANG STRING) WITH (kafka_topic='test_topic', value_format='JSON');
CREATE TABLE TWEET_LANG AS SELECT TIMESTAMPTOSTRING(WINDOWSTART(),'yyyy-MM-dd HH:mm:ss Z', 'UTC') AS WSTART, TIMESTAMPTOSTRING(WINDOWEND(),'yyyy-MM-dd HH:mm:ss Z', 'UTC') AS WEND, LANG, COUNT(*) AS TWEET_COUNT FROM ALL_TWEETS WINDOW TUMBLING (SIZE 1 SECOND) GROUP BY LANG;

-- pull query:
SELECT WSTART, WEND, LANG, TWEET_COUNT FROM TWEET_LANG WHERE ROWKEY='en';
```

Before this change the pull query would return `null` values for `WSTART` and `WEND`, even though the data was correctly populated in the `TWEET_LANG` topic. With this change the correct values for `WSTART` and `WEND` are returned.

The main fix is in `StreamAggregateBuilder`, which now applies a suitable `map` call that will apply the special processing to any pull query.
@big-andy-coates big-andy-coates requested a review from a team as a code owner February 4, 2020 16:18
Conflicting files
ksql-rest-app/src/test/java/io/confluent/ksql/rest/entity/TableRowsEntityFactoryTest.java
ksql-streams/src/test/java/io/confluent/ksql/execution/streams/materialization/WindowedRowTest.java
Conflicting files
ksql-execution/src/test/java/io/confluent/ksql/execution/transform/window/WindowSelectMapperTest.java
ksql-rest-app/src/test/java/io/confluent/ksql/rest/entity/TableRowsEntityFactoryTest.java
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@big-andy-coates big-andy-coates merged commit 8da2b63 into confluentinc:master Feb 4, 2020
@big-andy-coates big-andy-coates deleted the fix_windowstart_pull_queries branch February 4, 2020 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ksqlDB table incorrectly returns window-based field as null in pull query
2 participants