read_side_padding ignores the pad string for dictionary inputs in lpad and rpad #7453

Activity

1. github-actions

   added

   requires-triage

   on Nov 3, 2026
2. andygrove

   added

   bugSomething isn't workingSomething isn't working

   priority:criticalData corruption, silent wrong results, security issuesData corruption, silent wrong results, security issues

   area:expressionsExpression evaluationExpression evaluation

   and removed

   requires-triage

   on Nov 9, 2026
3. kazuyukitanimura

   added

   priority:highCrashes, panics, segfaults, major functional breakageCrashes, panics, segfaults, major functional breakage

   and removed

   priority:criticalData corruption, silent wrong results, security issuesData corruption, silent wrong results, security issues

   on Nov 9, 2026
4. kazuyukitanimura commented on Nov 9, 2026

   

   kazuyukitanimura

   on Nov 9, 2026

   Contributor

   More actions

   Hi @tlfua
   Thank you for reporting. Would you mind checking if this bug is reachable? It could be we are always converting to non-dictionary...

   👍React with 👍1Reacted by Tien-Lung Fu
5. tlfua commented on Nov 10, 2026

   

   tlfua

   on Nov 10, 2026

   Author

   More actions

   Thanks @kazuyukitanimura, you're right. I traced every path that feeds a native expression on main (8c145b7), and none of them can hand read_side_padding a dictionary array today:

   • JVM input (ScanExec, which covers JVM shuffle reads, broadcasts and local table scans): schema_from_data_types unpacks every dictionary type, and build_record_batch casts each column to that schema.
     

     datafusion-comet/native/core/src/execution/operators/scan.rs

     Lines 195 to 208 in 8c145b7

     	/// The schema of an input leaf whose columns have `data_types`, without dictionary types.
     	pub(crate) fn schema_from_data_types(data_types: &[DataType]) -> SchemaRef {
     	let fields = data_types
     	.iter()
     	.enumerate()
     	.map(|(idx, dt)| {
     	let datatype = ScanExec::unpack_dictionary_type(dt);
     	// We don't use the field name. Put a placeholder.
     	Field::new(format!("col_{idx}"), datatype, true)
     	})
     	.collect::<Vec<Field>>();
     	Arc::new(Schema::new(fields))
     	}

     
     

     datafusion-comet/native/core/src/execution/operators/scan.rs

     Lines 309 to 333 in 8c145b7

     	fn build_record_batch(
     	&self,
     	columns: &[ArrayRef],
     	num_rows: usize,
     	) -> DataFusionResult<RecordBatch, DataFusionError> {
     	let schema_fields = self.schema.fields();
     	assert_eq!(columns.len(), schema_fields.len());
     	// Cast dictionary-encoded primitive arrays to regular arrays and cast
     	// Utf8/LargeUtf8/Binary arrays to dictionary-encoded if the schema is
     	// defined as dictionary-encoded and the data in this batch is not
     	// dictionary-encoded (could also be the other way around)
     	let new_columns: Vec<ArrayRef> = columns
     	.iter()
     	.zip(schema_fields.iter())
     	.enumerate()
     	.map(|(idx, (column, f))| {
     	if column.data_type() != f.data_type() {
     	self.check_timestamp_labels(idx, f, column)?;
     	let mut timer = self.cast_time.timer();
     	let cast_array = cast_with_options(column, f.data_type(), &self.cast_options);
     	timer.stop();
     	cast_array.map_err(DataFusionError::from)
     	} else {
     	Ok(Arc::clone(column))

   • Native shuffle read unpacks dictionaries before handing batches on.
     

     datafusion-comet/native/core/src/execution/operators/shuffle_scan.rs

     Lines 221 to 229 in 8c145b7

     	// Extract column arrays, unpacking any dictionary-encoded columns.
     	// Native shuffle may dictionary-encode string/binary columns for efficiency,
     	// but downstream DataFusion operators expect the value types declared in the
     	// schema (e.g. Utf8, not Dictionary<Int32, Utf8>).
     	let columns: Vec<ArrayRef> = batch
     	.columns()
     	.iter()
     	.map(|col| unpack_dictionary(col))
     	.collect();

   • Native Parquet and Iceberg scans adapt to the required Spark schema, which unwraps dictionary columns even when the file's ARROW:schema declares them. This is covered by dictionary_columns_are_unwrapped_to_required_types.
     

     datafusion-comet/native/core/src/parquet/schema_adapter.rs

     Line 3121 in 8c145b7

     .into_iter()

     
     

     datafusion-comet/native/core/src/execution/operators/iceberg_scan.rs

     Line 240 in 8c145b7

     let adapter_factory = SparkPhysicalExprAdapterFactory::new(spark_options, None);

   • Target types come from to_arrow_datatype, which maps StringType to Utf8 and never to Dictionary.
     

     datafusion-comet/native/core/src/execution/serde.rs

     Line 80 in 8c145b7

     String => ArrowDataType::Utf8,

   • Other expressions: no non-test code in native/ builds a dictionary array from a plain one. The only place that creates one is read_side_padding itself, and only when its input is already a dictionary.

   The arm was reachable when it was written. #1482 added the Dictionary arm, and #2436 copied it into the three-argument case with SPACE. At that time ScanExec still kept string dictionaries. #2635 then changed ScanExec to unpack every dictionary type, which made the arm dead code.
   

   datafusion-comet/native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs

   Lines 113 to 137 in 8c145b7

   	),
   	// Dictionary support required for SPARK-48498
   	DataType::Dictionary(_, value_type) => {
   	let dict = as_dictionary_array::<Int32Type>(array);
   	let col = if value_type.as_ref() == &DataType::Utf8 {
   	spark_read_side_padding_internal::<i32>(
   	dict.values(),
   	truncate,
   	ColumnarValue::Scalar(ScalarValue::Int32(Some(*length))),
   	SPACE,
   	is_left_pad,
   	)?
   	} else {
   	spark_read_side_padding_internal::<i64>(
   	dict.values(),
   	truncate,
   	ColumnarValue::Scalar(ScalarValue::Int32(Some(*length))),
   	SPACE,
   	is_left_pad,
   	)?
   	};
   	// col consists of an array, so arg of to_array() is not used. Can be anything
   	let values = col.to_array(0)?;
   	let result = DictionaryArray::try_new(dict.keys().clone(), values)?;
   	Ok(ColumnarValue::Array(make_array(result.into())))

   So this is latent rather than a user-facing wrong result, and I don't think it should be priority:critical. I'll leave the exact priority to the maintainers. I'm happy to send a small PR that passes the pad string in both Dictionary branches and adds a kernel unit test, so the arm stays correct if a dictionary ever reaches it again. Would you prefer that, or should I close this as not reachable?