Skip to content

Add @JsonProperty tag to ignoreNulls flag so it works in distributed …#14221

Merged
rongrong merged 1 commit intoprestodb:masterfrom
kaikalur:fix_ignore_nulls
Mar 12, 2020
Merged

Add @JsonProperty tag to ignoreNulls flag so it works in distributed …#14221
rongrong merged 1 commit intoprestodb:masterfrom
kaikalur:fix_ignore_nulls

Conversation

@kaikalur
Copy link
Contributor

@kaikalur kaikalur commented Mar 6, 2020

Add jsonproperty tag to ignoreNulls so it works in distributed execution as well.

== RELEASE NOTES ==

Fixes a bug in window functions with IGNORE NULLS not working properly in a distributed environment as the flag is not serialized propery.

@kaikalur kaikalur requested a review from rongrong March 6, 2020 00:32
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably write it similar as other tests using assertQueryOrdered and provide expected results, rather than just checking for null (look at testValueWindowFunctions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not need ordered. So testing for unordered

@rongrong
Copy link
Contributor

rongrong commented Mar 6, 2020

There probably should be a release note since this is a fix.

@kaikalur kaikalur force-pushed the fix_ignore_nulls branch 2 times, most recently from 7471800 to 6320e0c Compare March 8, 2020 16:58
@kaikalur
Copy link
Contributor Author

kaikalur commented Mar 9, 2020

There probably should be a release note since this is a fix.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nits: separate line for new argument. And empty space between values.

    "FROM T",
    "Values 1, 2, 2");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

Nits: new line for argument.

    assertQuery(
        "WITH T ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Don't know why 'Reformat Code' doesn't do it automatically though

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think our intelij style setup really conforms with the actual style we are following :(

@kaikalur kaikalur force-pushed the fix_ignore_nulls branch 2 times, most recently from 7ce1e15 to ab76ffa Compare March 12, 2020 04:31
…execution as well.

Simplified the test.
Formatting.
@rongrong rongrong merged commit 39c43b6 into prestodb:master Mar 12, 2020
@caithagoras caithagoras mentioned this pull request Mar 29, 2020
9 tasks
@caithagoras caithagoras mentioned this pull request May 4, 2020
34 tasks
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.

2 participants