Skip to content

Conversation

@cloud-fan
Copy link
Contributor

In #7752 we added FromUnsafe to convert nexted unsafe data like array/map/struct to safe versions. It's a quick solution and we already have GenerateSafe to do the conversion which is codegened. So we should remove FromUnsafe and implement its codegen version in GenerateSafe.

@cloud-fan
Copy link
Contributor Author

cc @davies @rxin

@SparkQA
Copy link

SparkQA commented Aug 7, 2015

Test build #40165 has finished for PR 8029 at commit a93fd4b.

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

@cloud-fan
Copy link
Contributor Author

retest this please.

@SparkQA
Copy link

SparkQA commented Aug 7, 2015

Test build #40166 has finished for PR 8029 at commit a93fd4b.

  • This patch passes all 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.

This could be used in generated MutableProjection, so we need the copy() here, or we should make sure that UnsafeRow will not be used with MutableProjection (the fallback version of new aggregation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the copy here, just want to tighten up the scope that we only need to do copy when convert a unsafe row to safe row(according to the fact that we only copy nested struct in GenerateSafe not here before).

Copy link
Contributor

Choose a reason for hiding this comment

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

SafeProjection may be not the only one projection that turn an UnsafeRow into InternalRow. For some operators that accept both safe row and unsafe row, that will use generated mutable projection, it will got crupted UTF8String if we remove the copy here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, thanks for your explanation!

@davies
Copy link
Contributor

davies commented Aug 7, 2015

LGTM, except the UTF8String change, we could leave that our of this PR and investigate more.

@davies
Copy link
Contributor

davies commented Aug 7, 2015

Could you fill the description a few words?

@SparkQA
Copy link

SparkQA commented Aug 8, 2015

Test build #40234 has finished for PR 8029 at commit ed40d8f.

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

@davies
Copy link
Contributor

davies commented Aug 8, 2015

LGTM, merging this into master and 1.5, thanks!

asfgit pushed a commit that referenced this pull request Aug 8, 2015
…enerateSafe

In #7752 we added `FromUnsafe` to convert nexted unsafe data like array/map/struct to safe versions. It's a quick solution and we already have `GenerateSafe` to do the conversion which is codegened. So we should remove `FromUnsafe` and implement its codegen version in `GenerateSafe`.

Author: Wenchen Fan <[email protected]>

Closes #8029 from cloud-fan/from-unsafe and squashes the following commits:

ed40d8f [Wenchen Fan] add the copy back
a93fd4b [Wenchen Fan] cogengen FromUnsafe

(cherry picked from commit 106c078)
Signed-off-by: Davies Liu <[email protected]>
@asfgit asfgit closed this in 106c078 Aug 8, 2015
@cloud-fan cloud-fan deleted the from-unsafe branch August 8, 2015 18:26
CodingCat pushed a commit to CodingCat/spark that referenced this pull request Aug 17, 2015
…enerateSafe

In apache#7752 we added `FromUnsafe` to convert nexted unsafe data like array/map/struct to safe versions. It's a quick solution and we already have `GenerateSafe` to do the conversion which is codegened. So we should remove `FromUnsafe` and implement its codegen version in `GenerateSafe`.

Author: Wenchen Fan <[email protected]>

Closes apache#8029 from cloud-fan/from-unsafe and squashes the following commits:

ed40d8f [Wenchen Fan] add the copy back
a93fd4b [Wenchen Fan] cogengen FromUnsafe
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