Skip to content

Fix ARRAY_DISTINCT wrong results on NULL and 0#12103

Merged
yingsu00 merged 1 commit intoprestodb:masterfrom
yingsu00:arraydistinct_fix
Dec 19, 2018
Merged

Fix ARRAY_DISTINCT wrong results on NULL and 0#12103
yingsu00 merged 1 commit intoprestodb:masterfrom
yingsu00:arraydistinct_fix

Conversation

@yingsu00
Copy link
Contributor

Resolves #11977
Breaking #12080 into separate PRs. This PR is to fix ARRAY_DISTINCT only.

assertFunction("ARRAY_DISTINCT(ARRAY [])", new ArrayType(UNKNOWN), ImmutableList.of());

// Order matters here. Result should be stable.
assertFunction("ARRAY_DISTINCT(ARRAY [0, NULL])", new ArrayType(INTEGER), asList(0, (Object) null));
Copy link
Contributor

@rongrong rongrong Dec 19, 2018

Choose a reason for hiding this comment

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

I don't think the (Object) is needed here, as well as in other newly added tests.


if (array.getPositionCount() == 2) {
if (type.equalTo(array, 0, array, 1)) {
if (TypeUtils.positionEqualsPosition(type, array, 0, array, 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember you mentioned we could use OperatorDependency to get the equal MethodHandle -- is that feasible?

I am just asking and this doesn't need to be done in this PR. -- we migrate the usage of positionEqualsPosition in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wenleix we discussed to change the equal semantics to isDistinctFrom. I've been waiting for #11983 to get this done. It'll be in seperate PR.

Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

Looks good . Remember to address @rongrong 's comment about test before merge.

@yingsu00 yingsu00 merged commit 1dec59f into prestodb:master Dec 19, 2018
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.

ARRAY_DISTINCT returns wrong results

4 participants