-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-40918][SQL] Mismatch between FileSourceScanExec and Orc and ParquetFileFormat on producing columnar output #38397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
918c65b
d805162
6b851c2
4e6729e
ab83b1c
d3ff82d
4fc51e7
e36bdee
e6211fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,6 @@ import org.apache.spark.sql.SparkSession | |
| import org.apache.spark.sql.catalyst.InternalRow | ||
| import org.apache.spark.sql.catalyst.expressions._ | ||
| import org.apache.spark.sql.catalyst.expressions.codegen.GenerateUnsafeProjection | ||
| import org.apache.spark.sql.execution.WholeStageCodegenExec | ||
| import org.apache.spark.sql.execution.datasources._ | ||
| import org.apache.spark.sql.sources._ | ||
| import org.apache.spark.sql.types._ | ||
|
|
@@ -102,8 +101,7 @@ class OrcFileFormat | |
|
|
||
| override def supportBatch(sparkSession: SparkSession, schema: StructType): Boolean = { | ||
| val conf = sparkSession.sessionState.conf | ||
| conf.orcVectorizedReaderEnabled && conf.wholeStageEnabled && | ||
| !WholeStageCodegenExec.isTooManyFields(conf, schema) && | ||
| conf.orcVectorizedReaderEnabled && | ||
| schema.forall(s => OrcUtils.supportColumnarReads( | ||
| s.dataType, sparkSession.sessionState.conf.orcVectorizedReaderNestedColumnEnabled)) | ||
| } | ||
|
|
@@ -115,6 +113,18 @@ class OrcFileFormat | |
| true | ||
| } | ||
|
|
||
| /** | ||
| * Build the reader. | ||
| * | ||
| * @note It is required to pass FileFormat.OPTION_RETURNING_BATCH in options, to indicate whether | ||
| * the reader should return row or columnar output. | ||
| * If the caller can handle both, pass | ||
| * FileFormat.OPTION_RETURNING_BATCH -> | ||
| * supportBatch(sparkSession, | ||
| * StructType(requiredSchema.fields ++ partitionSchema.fields)) | ||
| * as the option. | ||
| * It should be set to "true" only if this reader can support it. | ||
| */ | ||
| override def buildReaderWithPartitionValues( | ||
| sparkSession: SparkSession, | ||
| dataSchema: StructType, | ||
|
|
@@ -126,9 +136,24 @@ class OrcFileFormat | |
|
|
||
| val resultSchema = StructType(requiredSchema.fields ++ partitionSchema.fields) | ||
| val sqlConf = sparkSession.sessionState.conf | ||
| val enableVectorizedReader = supportBatch(sparkSession, resultSchema) | ||
| val capacity = sqlConf.orcVectorizedReaderBatchSize | ||
|
|
||
| // Should always be set by FileSourceScanExec creating this. | ||
| // Check conf before checking option, to allow working around an issue by changing conf. | ||
| val enableVectorizedReader = sqlConf.orcVectorizedReaderEnabled && | ||
| options.get(FileFormat.OPTION_RETURNING_BATCH) | ||
| .getOrElse { | ||
| throw new IllegalArgumentException( | ||
| "OPTION_RETURNING_BATCH should always be set for OrcFileFormat." + | ||
| "To workaround this issue, set spark.sql.orc.enableVectorizedReader=false.") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a correct recommendation? Why not recommend to set
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun passing OPTION_RETURNING_BATCH is something that the developer of the code that called without setting this option can do. For an end user who faces this issue by hitting some code path that doesn't set this, the workaround would be to disable this config. Hence it's called a "workaround" not a "fix". |
||
| } | ||
| .equals("true") | ||
| if (enableVectorizedReader) { | ||
| // If the passed option said that we are to return batches, we need to also be able to | ||
| // do this based on config and resultSchema. | ||
| assert(supportBatch(sparkSession, resultSchema)) | ||
| } | ||
|
|
||
| OrcConf.IS_SCHEMA_EVOLUTION_CASE_SENSITIVE.setBoolean(hadoopConf, sqlConf.caseSensitiveAnalysis) | ||
|
|
||
| val broadcastedConf = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,6 @@ import org.apache.spark.sql.catalyst.expressions.codegen.GenerateUnsafeProjectio | |
| import org.apache.spark.sql.catalyst.parser.LegacyTypeStringParser | ||
| import org.apache.spark.sql.catalyst.util.DateTimeUtils | ||
| import org.apache.spark.sql.errors.QueryExecutionErrors | ||
| import org.apache.spark.sql.execution.WholeStageCodegenExec | ||
| import org.apache.spark.sql.execution.datasources._ | ||
| import org.apache.spark.sql.execution.vectorized.{ConstantColumnVector, OffHeapColumnVector, OnHeapColumnVector} | ||
| import org.apache.spark.sql.internal.SQLConf | ||
|
|
@@ -82,12 +81,11 @@ class ParquetFileFormat | |
| } | ||
|
|
||
| /** | ||
| * Returns whether the reader will return the rows as batch or not. | ||
| * Returns whether the reader can return the rows as batch or not. | ||
| */ | ||
| override def supportBatch(sparkSession: SparkSession, schema: StructType): Boolean = { | ||
| val conf = sparkSession.sessionState.conf | ||
| ParquetUtils.isBatchReadSupportedForSchema(conf, schema) && conf.wholeStageEnabled && | ||
| !WholeStageCodegenExec.isTooManyFields(conf, schema) | ||
| ParquetUtils.isBatchReadSupportedForSchema(conf, schema) | ||
| } | ||
|
|
||
| override def vectorTypes( | ||
|
|
@@ -110,6 +108,18 @@ class ParquetFileFormat | |
| true | ||
| } | ||
|
|
||
| /** | ||
| * Build the reader. | ||
| * | ||
| * @note It is required to pass FileFormat.OPTION_RETURNING_BATCH in options, to indicate whether | ||
| * the reader should return row or columnar output. | ||
| * If the caller can handle both, pass | ||
| * FileFormat.OPTION_RETURNING_BATCH -> | ||
| * supportBatch(sparkSession, | ||
| * StructType(requiredSchema.fields ++ partitionSchema.fields)) | ||
| * as the option. | ||
| * It should be set to "true" only if this reader can support it. | ||
| */ | ||
| override def buildReaderWithPartitionValues( | ||
| sparkSession: SparkSession, | ||
| dataSchema: StructType, | ||
|
|
@@ -161,8 +171,6 @@ class ParquetFileFormat | |
| val timestampConversion: Boolean = sqlConf.isParquetINT96TimestampConversion | ||
| val capacity = sqlConf.parquetVectorizedReaderBatchSize | ||
| val enableParquetFilterPushDown: Boolean = sqlConf.parquetFilterPushDown | ||
| // Whole stage codegen (PhysicalRDD) is able to deal with batches directly | ||
| val returningBatch = supportBatch(sparkSession, resultSchema) | ||
| val pushDownDate = sqlConf.parquetFilterPushDownDate | ||
| val pushDownTimestamp = sqlConf.parquetFilterPushDownTimestamp | ||
| val pushDownDecimal = sqlConf.parquetFilterPushDownDecimal | ||
|
|
@@ -173,6 +181,22 @@ class ParquetFileFormat | |
| val datetimeRebaseModeInRead = parquetOptions.datetimeRebaseModeInRead | ||
| val int96RebaseModeInRead = parquetOptions.int96RebaseModeInRead | ||
|
|
||
| // Should always be set by FileSourceScanExec creating this. | ||
| // Check conf before checking option, to allow working around an issue by changing conf. | ||
| val returningBatch = sparkSession.sessionState.conf.parquetVectorizedReaderEnabled && | ||
| options.get(FileFormat.OPTION_RETURNING_BATCH) | ||
| .getOrElse { | ||
| throw new IllegalArgumentException( | ||
| "OPTION_RETURNING_BATCH should always be set for ParquetFileFormat." + | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. nit. Add one more space at the end of the message. |
||
| "To workaround this issue, set spark.sql.parquet.enableVectorizedReader=false.") | ||
| } | ||
| .equals("true") | ||
| if (returningBatch) { | ||
| // If the passed option said that we are to return batches, we need to also be able to | ||
| // do this based on config and resultSchema. | ||
| assert(supportBatch(sparkSession, resultSchema)) | ||
| } | ||
|
|
||
| (file: PartitionedFile) => { | ||
| assert(file.partitionValues.numFields == partitionSchema.size) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. Add one space at the end?