Skip to content

Fixed incorrect asserts#2627

Closed
rendel wants to merge 1 commit intoapache:mainfrom
rendel:patch-1
Closed

Fixed incorrect asserts#2627
rendel wants to merge 1 commit intoapache:mainfrom
rendel:patch-1

Conversation

@rendel
Copy link

@rendel rendel commented Dec 2, 2021

It is not possible to use a range distribution with the current assert, it will always fail.

It is not possible to use a range distribution with the current assert, it will always fail.
|| type == Type.RANDOM_DISTRIBUTED
|| type == Type.RANGE_DISTRIBUTED
|| keys.isEmpty();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@rendel Thank you for reporting this. I think it is a bug. Could you file a JIRA for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say a test case is more important than JIRA.
If the assertion is user-facing, then it should be a regular if + illegalargumentexception

@chunweilei chunweilei added the needs-a-final-review This patch looks good from at least one PMC or committer, but still needs a specialist final review label Dec 10, 2021
@chunweilei
Copy link
Contributor

BTW, you can find more details about contribution in the doc[1].

[1] https://calcite.apache.org/develop/#contributing

@chunweilei
Copy link
Contributor

Let us move to #2822.

@chunweilei chunweilei closed this May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-a-final-review This patch looks good from at least one PMC or committer, but still needs a specialist final review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants