cloud-fan commented on code in PR #56991:
URL: https://github.com/apache/spark/pull/56991#discussion_r3523376529
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/types/ops/ParquetTypeOps.scala:
##########
@@ -214,6 +214,18 @@ private[parquet] trait ParquetTypeOps extends Serializable
{
* @param descriptor the Parquet column descriptor being read
*/
def getVectorUpdater(descriptor: ColumnDescriptor):
Option[ParquetVectorUpdater] = None
+
+ /**
+ * Whether a dictionary-encoded column of this type can be lazily
dictionary-decoded on the
+ * vectorized path (the dictionary is attached to the column vector and
decoded on read) rather
+ * than eagerly decoding every value up front. Default true. Override to
false when the
+ * vectorized updater ([[getVectorUpdater]]) does per-value processing -
unit conversion,
+ * truncation, rebasing - that lazy decoding would bypass. Consulted (Spark
DataType -> ops) by
+ * `VectorizedColumnReader.isLazyDecodingSupported`.
+ *
+ * @param descriptor the Parquet column descriptor being read
+ */
+ def supportsLazyDictionaryDecoding(descriptor: ColumnDescriptor): Boolean =
true
Review Comment:
Why default to `true` (opt-out) here? The built-in switch this replaces
starts from `isSupported = false` and opts specific cases in, and the sibling
`isBatchReadSupported` gate in this same trait defaults `false` ("types must
opt in"). A `true` default inverts that stance: a future framework primitive
type that opts into vectorized reads (`isBatchReadSupported = true`) with a
per-value updater — unit conversion / truncation / rebase, exactly TimeType's
situation — but that forgets the `false` override would silently get lazy
dictionary decoding, bypass the per-value work, and return wrong results with
no error. A `false` default would fail-safe (at worst a missed lazy-decode
optimization). Not blocking — no current type is affected and the Scaladoc
documents the override — but worth a sentence on why `true` was chosen over
matching `isBatchReadSupported`'s opt-in default, or flipping it to `false`.
--
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]
