Skip to content

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

These dictionary related APIs are special to WritableColumnVector and should not be in ColumnVector, which will be public soon.

How was this patch tested?

existing tests

@cloud-fan
Copy link
Contributor Author

cc @kiszk @ueshin

@SparkQA
Copy link

SparkQA commented Dec 13, 2017

Test build #84871 has finished for PR 19970 at commit 103dca3.

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

@gatorsmile
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Dec 14, 2017

Test build #84881 has finished for PR 19970 at commit 103dca3.

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

Copy link
Member

@ueshin ueshin left a comment

Choose a reason for hiding this comment

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

We can simplify reserveDictionaryIds() at WritableColumnVector.java#L659 as well.

Otherwise, LGTM.

/**
* Returns a utility object to get structs.
* provided to keep API compatibility with InternalRow for code generation
* A special version of {@link #getShort(int)}, which is only used as an adapter for Spark codegen
Copy link
Member

Choose a reason for hiding this comment

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

getStruct(int) instead of getShort(int)?

@SparkQA
Copy link

SparkQA commented Dec 14, 2017

Test build #84891 has finished for PR 19970 at commit c38f58e.

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

@cloud-fan
Copy link
Contributor Author

retest this pease


/**
* Returns the dictionary Id for rowId.
* This should only be called when the ColumnVector is dictionaryIds.
Copy link
Member

Choose a reason for hiding this comment

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

ColumnVector -> WritableColumnVector

Copy link
Member

@kiszk kiszk Dec 14, 2017

Choose a reason for hiding this comment

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

when dictionaryIds has WritableColumnVector.?

@kiszk
Copy link
Member

kiszk commented Dec 14, 2017

LGTM except a few comments for wording

@SparkQA
Copy link

SparkQA commented Dec 14, 2017

Test build #84903 has finished for PR 19970 at commit 1de8de9.

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

@cloud-fan
Copy link
Contributor Author

thanks, merging to master!

@asfgit asfgit closed this in 7d8e2ca Dec 14, 2017
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.

5 participants