LuciferYang commented on code in PR #12954:
URL: https://github.com/apache/gluten/pull/12954#discussion_r3950240505
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -89,33 +91,40 @@ trait SparkShims {
sparkSession: SparkSession,
readFunction: PartitionedFile => Iterator[InternalRow],
filePartitions: Seq[FilePartition],
- fileSourceScanExec: FileSourceScanExec): FileScanRDD
+ fileSourceScanExec: FileSourceScanExec): FileScanRDD = {
+ new FileScanRDD(
+ sparkSession,
+ readFunction,
+ filePartitions,
+ new StructType(
+ fileSourceScanExec.requiredSchema.fields ++
+ fileSourceScanExec.relation.partitionSchema.fields),
+ fileSourceScanExec.fileConstantMetadataColumns
+ )
+ }
def filesGroupedToBuckets(
selectedPartitions: Array[PartitionDirectory]): Map[Int,
Array[PartitionedFile]]
- // Spark3.4 new add table parameter in BatchScanExec.
- def getBatchScanExecTable(batchScan: BatchScanExec): Table
+ def getBatchScanExecTable(batchScan: BatchScanExec): Table = batchScan.table
- // The PartitionedFile API changed in spark 3.4
def generatePartitionedFile(
partitionValues: InternalRow,
filePath: String,
start: Long,
length: Long,
- @transient locations: Array[String] = Array.empty): PartitionedFile
+ @transient locations: Array[String] = Array.empty): PartitionedFile =
+ PartitionedFile(partitionValues, SparkPath.fromPathString(filePath),
start, length, locations)
Review Comment:
Removed. The callers are `SoftAffinitySuite` and
`SoftAffinityWithRDDInfoSuite`, 12 sites between them, so per the second half
of your criterion each suite got a small private `partitionedFile(path, start,
length, locations)` rather than 12 copies of the constructor call.
`PartitionsUtilSuite` in the same tree already uses that pattern.
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -89,33 +91,40 @@ trait SparkShims {
sparkSession: SparkSession,
readFunction: PartitionedFile => Iterator[InternalRow],
filePartitions: Seq[FilePartition],
- fileSourceScanExec: FileSourceScanExec): FileScanRDD
+ fileSourceScanExec: FileSourceScanExec): FileScanRDD = {
+ new FileScanRDD(
+ sparkSession,
+ readFunction,
+ filePartitions,
+ new StructType(
+ fileSourceScanExec.requiredSchema.fields ++
+ fileSourceScanExec.relation.partitionSchema.fields),
+ fileSourceScanExec.fileConstantMetadataColumns
Review Comment:
Removed, and it turned out to have no production caller at all: the only
reference in the tree was `CHAggAndShuffleBenchmark`, which now builds the
`FileScanRDD` itself. The first five constructor parameters are identical on
3.4 through 4.1, so the direct construction is version-safe; 3.5+ inserts
`metadataExtractors` at position six, which is worth knowing before anyone adds
a sixth positional argument.
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
Review Comment:
Done, all of it. Fifteen methods leave the trait now (64 down to 46):
fourteen inlined at the call site, and `withTryEvalMode` / `withAnsiEvalMode` /
`generateMetadataColumns` moved into `gluten-substrait` utils since each has
two callers. The PR description has the full list and the per-method reasoning;
individual replies are on your comments above.
Two of them look removable and are not, so they stay with a note in #12953:
`createParquetFilters` needs `LegacyBehaviorPolicy`, which is nested in
`SQLConf` on 3.4 and top-level from 3.5 on, so no single import in a shared
module compiles against all four versions; and `widerDecimalType` forwards to
`DecimalPrecision` on 3.4/3.5 and `DecimalPrecisionTypeCoercion` on 4.0/4.1.
Both become removable when 3.4 goes.
One behavior change worth your eye: `ExpressionConverter` matched
`ArrayInsert` through `getClass.getSimpleName` because the class did not exist
on 3.3. With the cast now inlined next to it that guard was self-contradictory,
so it is a plain type match. A third-party class whose simple name is also
`ArrayInsert` used to enter the arm and fail in the cast; it now falls through.
Verification is compile-only, no suites: substrait including test sources on
all four versions, velox on 3.5/4.0/4.1, iceberg on 3.4/3.5, paimon on 3.5,
clickhouse on 3.5, and `spotless:check` on all four. The gaps and why they are
gaps are in the description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
