sarutak commented on code in PR #59125:
URL: https://github.com/apache/spark/pull/59125#discussion_r4130240567
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetUtils.scala:
##########
@@ -361,16 +362,18 @@ object ParquetUtils extends Logging {
case count: Count if
V2ColumnUtils.extractV2Column(count.column).isDefined =>
val colName = V2ColumnUtils.extractV2Column(count.column).get
schemaName = "count(" + colName + ")"
- rowCount += block.getRowCount
- var isPartitionCol = false
- if (partitionSchema.getFieldIndex(colName).isDefined) {
- isPartitionCol = true
- }
isCount = true
- if (!isPartitionCol) {
+ val partitionColIndex = partitionSchema.getFieldIndex(colName)
+ if (partitionColIndex.isEmpty) {
+ rowCount += block.getRowCount
index = dataSchema.getFieldIndex(colName).getOrElse(-1)
// Count(*) includes the null values, but Count(colName) doesn't.
rowCount -= getNumNulls(filePath, blockMetaData, index)
+ } else if (!partitionValues.isNullAt(partitionColIndex.get)) {
Review Comment:
`partitionColIndex.get` is only reached in the `else` of `if (isEmpty)`, so
it is safe, but it is non-idiomatic and asymmetric with the ORC side (which
binds the index in the `match` and never calls `.get`). Folding it into a
`match` (Comment 1) removes the `.get` naturally.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetUtils.scala:
##########
@@ -361,16 +362,18 @@ object ParquetUtils extends Logging {
case count: Count if
V2ColumnUtils.extractV2Column(count.column).isDefined =>
val colName = V2ColumnUtils.extractV2Column(count.column).get
schemaName = "count(" + colName + ")"
- rowCount += block.getRowCount
- var isPartitionCol = false
- if (partitionSchema.getFieldIndex(colName).isDefined) {
- isPartitionCol = true
- }
isCount = true
- if (!isPartitionCol) {
+ val partitionColIndex = partitionSchema.getFieldIndex(colName)
Review Comment:
The two engines now express the same three-way branch differently: ORC uses
a `match` (`case Some(index) if partitionValues.isNullAt(index) => 0L` / `case
Some(_)` / `case None`), while Parquet uses `if (partitionColIndex.isEmpty) …
else if (!isNullAt) …`. Both are correct, but making Parquet mirror the ORC
`match` would keep the two paths visually parallel and easier to maintain in
lockstep.
The following is an example for Parquet, mirroring the ORC match:
```scala
partitionSchema.getFieldIndex(colName) match {
case None =>
rowCount += block.getRowCount
index = dataSchema.getFieldIndex(colName).getOrElse(-1)
// Count(*) includes the null values, but Count(colName) doesn't.
rowCount -= getNumNulls(filePath, blockMetaData, index)
// A null partition (__HIVE_DEFAULT_PARTITION__) is SQL NULL and
contributes 0.
case Some(i) if !partitionValues.isNullAt(i) => rowCount +=
block.getRowCount
case Some(_) => // null partition: contribute 0
}
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetUtils.scala:
##########
@@ -361,16 +362,18 @@ object ParquetUtils extends Logging {
case count: Count if
V2ColumnUtils.extractV2Column(count.column).isDefined =>
val colName = V2ColumnUtils.extractV2Column(count.column).get
schemaName = "count(" + colName + ")"
- rowCount += block.getRowCount
- var isPartitionCol = false
- if (partitionSchema.getFieldIndex(colName).isDefined) {
- isPartitionCol = true
- }
isCount = true
- if (!isPartitionCol) {
+ val partitionColIndex = partitionSchema.getFieldIndex(colName)
Review Comment:
In the Parquet path, `partitionColIndex` and `partitionValues.isNullAt(...)`
are re-evaluated on every block, but both are constant within a file (one
`PartitionedFile` = one partition). Computing them once before the block loop
is marginally cleaner; the runtime impact is negligible, so this is purely a
nit.
--
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]
