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.

Sign up for GitHub

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

Jump to bottom

Filter out nulls before group by and select key operations to avoid NPE #927

Merged
merged 5 commits into from
Mar 15, 2018
Merged

Filter out nulls before group by and select key operations to avoid NPE #927

merged 5 commits into from
Mar 15, 2018

Conversation

dguy
Copy link
Contributor

@dguy dguy commented Mar 13, 2018

Fixes #521

@dguy dguy requested a review from a team March 13, 2018 18:05
hjafarpour
hjafarpour approved these changes Mar 13, 2018
View reviewed changes
Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

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

LGTM.
This will avoid the NPE as stated but I think we should not have null value possibility here since all values are GenericRow instances. We have to see where we create the null value.

rodesai
rodesai reviewed Mar 13, 2018
View reviewed changes
ksql-engine/src/main/java/io/confluent/ksql/structured/SchemaKStream.java Outdated
@@ -240,7 +240,7 @@ public SchemaKStream selectKey(final Field newKeyField, boolean updateRowKey) {
}


KStream keyedKStream = kstream.selectKey((key, value) -> {
KStream keyedKStream = kstream.filter((key, value) -> value != null).selectKey((key, value) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're at it should we fix the NPE that I think would get thrown on line 248 if the value in the key column is null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @rodesai , I agree that it would be nice to fix it as part of this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. will do

@dguy
Copy link
Contributor Author

dguy commented Mar 14, 2018

@hjafarpour the null is returned from the Deserializers. The issue is that null is a valid value, i.e., for a compacted topic it signifies a delete for the key. So I think we should stick with filtering them for now and put some more thought into how we handle this post GA

big-andy-coates
big-andy-coates approved these changes Mar 14, 2018
View reviewed changes
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the cost of the double extractColumn is negligible.

ksql-engine/src/main/java/io/confluent/ksql/structured/SchemaKStream.java Outdated
@@ -133,7 +133,7 @@ public SchemaKStream select(final Schema selectSchema) {
List<Object> newColumns = new ArrayList<>();
for (Field schemaField : selectSchema.fields()) {
newColumns.add(
row.getColumns().get(SchemaUtil.getFieldIndexByName(schema, schemaField.name()))
extractColumn(schemaField, row)
Copy link
Contributor

@big-andy-coates big-andy-coates Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: all on a single line now?

apurvam
apurvam approved these changes Mar 14, 2018
View reviewed changes
Copy link
Contributor

@apurvam apurvam left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

rodesai
rodesai approved these changes Mar 15, 2018
View reviewed changes
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

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

LGTM

@dguy dguy merged commit d37e5dc into 4.1.x Mar 15, 2018
joel-hamill pushed a commit that referenced this pull request Mar 15, 2018
@dguy @joel-hamill
Filter out nulls before group by and select key operations to avoid N…
3c9988f 
…PE (#927)

Fixes #521
@big-andy-coates big-andy-coates deleted the ksql-521 branch March 20, 2018 12:53
@apurvam apurvam mentioned this pull request Mar 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apurvam apurvam apurvam approved these changes

@rodesai rodesai rodesai approved these changes

@big-andy-coates big-andy-coates big-andy-coates approved these changes

@hjafarpour hjafarpour hjafarpour approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dguy @apurvam @rodesai @big-andy-coates @hjafarpour