Skip to content

Conversation

@cloud-fan
Copy link
Contributor

Before this PR, user has to consume the iterator of one group before process next group, or we will get into infinite loops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we only use keyOrdering to do equality check, why not just use ==? The currentGroup and currentRow are from the same input, they must be both unsafe or safe, and == for UnsafeRow is faster than keyOrdering.compare.

cc @marmbrus

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 the whole row, not just the key. This allows us to do the equality check on the key columns only (which might short circuit) instead of doing a full projection on each row to extract the key columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry I missed it

@SparkQA
Copy link

SparkQA commented Oct 28, 2015

Test build #44518 has finished for PR 9330 at commit a8cc6b5.

  • This patch fails RAT tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are the same, and I prefer the idiomatic version.

@SparkQA
Copy link

SparkQA commented Oct 28, 2015

Test build #44529 has finished for PR 9330 at commit 9282e48.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 29, 2015

Test build #44560 has finished for PR 9330 at commit 28f959c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@marmbrus
Copy link
Contributor

Thanks, merging to master.

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.

3 participants