Skip to content

[SPARK-20783][SQL][Follow-up] Create ColumnVector to abstract existing compressed column#19508

Closed
viirya wants to merge 2 commits into
apache:masterfrom
viirya:SPARK-20783-followup
Closed

[SPARK-20783][SQL][Follow-up] Create ColumnVector to abstract existing compressed column#19508
viirya wants to merge 2 commits into
apache:masterfrom
viirya:SPARK-20783-followup

Conversation

@viirya

@viirya viirya commented Oct 16, 2017

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Removed one unused method.

How was this patch tested?

Existing tests.

@viirya

viirya commented Oct 16, 2017

Copy link
Copy Markdown
Member Author

cc @kiszk Please check if this change is proper.

@SparkQA

SparkQA commented Oct 16, 2017

Copy link
Copy Markdown

Test build #82804 has finished for PR 19508 at commit 5200ef0.

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

@viirya

viirya commented Oct 16, 2017

Copy link
Copy Markdown
Member Author

cc @cloud-fan too

@kiszk

kiszk commented Oct 17, 2017

Copy link
Copy Markdown
Member

I will check this on Wed.


protected def underlyingBuffer = buffer

def getByteBuffer: ByteBuffer =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch, it is not used.

columnType.dataType match {
case _: IntegerType =>
val dictionaryIds = columnVector.reserveDictionaryIds(capacity)
val intDictionary = dictionary.map(_.asInstanceOf[Int])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@viirya Could you please see this comment to avoid unboxing? IIUC, this code still causes unboxing.

What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't notice that comment. Let me check it then. If we can't avoid, I will revert this and only remove the unused method. Thanks.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @kiszk. I checked the comment and code and reverted this change.

@SparkQA

SparkQA commented Oct 19, 2017

Copy link
Copy Markdown

Test build #82897 has finished for PR 19508 at commit 25003cf.

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

@viirya

viirya commented Oct 19, 2017

Copy link
Copy Markdown
Member Author

ping @cloud-fan for final check. Thanks.

@kiszk

kiszk commented Oct 20, 2017

Copy link
Copy Markdown
Member

LGTM

@viirya

viirya commented Oct 22, 2017

Copy link
Copy Markdown
Member Author

ping @cloud-fan Please take a quick look. Thanks.

@viirya

viirya commented Oct 24, 2017

Copy link
Copy Markdown
Member Author

re-ping @cloud-fan This is a simple follow-up, please check it. Thanks.

@cloud-fan

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@asfgit asfgit closed this in 1051ebe Oct 25, 2017
@viirya viirya deleted the SPARK-20783-followup branch December 27, 2023 18:21
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.

4 participants