[SPARK-14237][SQL] De-duplicate partition value appending logic in various buildReader() implementations #12866
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
liancheng
force-pushed
the
spark-14237-simplify-partition-values-appending
branch
from
4c946a9 to
1f7ee5f
Compare
Contributor
Author
|
cc @yhuai @cloud-fan |
Contributor
Author
There was a problem hiding this comment.
@cloud-fan Instead of adding a new ReaderFunction trait with an initialize() method as you suggested, I used an anonymous Function1 class here. Not quite sure how useful the initialize() method can be in more general cases...
Contributor
There was a problem hiding this comment.
e.g. the text datasource, which need to initialize a UnsafeRowWriter for one reader function(not every file).
Contributor
Author
There was a problem hiding this comment.
That's a reasonable use case. But we can also use an anonymous Function1 class there.
|
Test build #57626 has finished for PR 12866 at commit
|
|
Test build #57628 has finished for PR 12866 at commit
|
liancheng
force-pushed
the
spark-14237-simplify-partition-values-appending
branch
from
1f7ee5f to
1bce7db
Compare
|
Test build #57702 has finished for PR 12866 at commit
|
Contributor
|
LGTM |
Contributor
Author
|
Thanks for the review! Merged this to master and branch-2.0. |
asfgit
pushed a commit
that referenced
this pull request
May 4, 2016
…rious buildReader() implementations ## What changes were proposed in this pull request? Currently, various `FileFormat` data sources share approximately the same code for partition value appending. This PR tries to eliminate this duplication. A new method `buildReaderWithPartitionValues()` is added to `FileFormat` with a default implementation that appends partition values to `InternalRow`s produced by the reader function returned by `buildReader()`. Special data sources like Parquet, which implements partition value appending inside `buildReader()` because of the vectorized reader, and the Text data source, which doesn't support partitioning, override `buildReaderWithPartitionValues()` and simply delegate to `buildReader()`. This PR brings two benefits: 1. Apparently, it de-duplicates partition value appending logic 2. Now the reader function returned by `buildReader()` is only required to produce `InternalRow`s rather than `UnsafeRow`s if the data source doesn't override `buildReaderWithPartitionValues()`. Because the safe-to-unsafe conversion is also performed while appending partition values. This makes 3rd-party data sources (e.g. spark-avro) easier to implement since they no longer need to access private APIs involving `UnsafeRow`. ## How was this patch tested? Existing tests should do the work. Author: Cheng Lian <lian@databricks.com> Closes #12866 from liancheng/spark-14237-simplify-partition-values-appending. (cherry picked from commit bc3760d) Signed-off-by: Cheng Lian <lian@databricks.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Currently, various
FileFormatdata sources share approximately the same code for partition value appending. This PR tries to eliminate this duplication.A new method
buildReaderWithPartitionValues()is added toFileFormatwith a default implementation that appends partition values toInternalRows produced by the reader function returned bybuildReader().Special data sources like Parquet, which implements partition value appending inside
buildReader()because of the vectorized reader, and the Text data source, which doesn't support partitioning, overridebuildReaderWithPartitionValues()and simply delegate tobuildReader().This PR brings two benefits:
Apparently, it de-duplicates partition value appending logic
Now the reader function returned by
buildReader()is only required to produceInternalRows rather thanUnsafeRows if the data source doesn't overridebuildReaderWithPartitionValues().Because the safe-to-unsafe conversion is also performed while appending partition values. This makes 3rd-party data sources (e.g. spark-avro) easier to implement since they no longer need to access private APIs involving
UnsafeRow.How was this patch tested?
Existing tests should do the work.