From 130143b0a405e3a44d62b244b066d2cc32f11169 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Mon, 5 Oct 2026 12:57:10 -0700 Subject: [PATCH 01/12] feat: txt file support --- .github/workflows/pr_build_linux.yml | 1 + .github/workflows/pr_build_macos.yml | 1 + docs/source/user-guide/latest/datasources.md | 11 + native/core/src/execution/jni_api.rs | 1 + native/core/src/execution/operators/mod.rs | 2 + .../core/src/execution/operators/text_scan.rs | 614 ++++++++++++++++++ native/core/src/execution/planner.rs | 41 ++ .../execution/planner/operator_registry.rs | 2 + native/proto/src/proto/operator.proto | 19 + .../scala/org/apache/comet/CometConf.scala | 11 + .../apache/comet/rules/CometExecRule.scala | 4 + .../apache/comet/rules/CometScanRule.scala | 144 +++- .../sql/comet/CometTextNativeScanExec.scala | 135 ++++ .../test/resources/test-data/text-test-1.txt | 6 + .../comet/text/CometTextNativeReadSuite.scala | 279 ++++++++ 15 files changed, 1270 insertions(+), 1 deletion(-) create mode 100644 native/core/src/execution/operators/text_scan.rs create mode 100644 spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala create mode 100644 spark/src/test/resources/test-data/text-test-1.txt create mode 100644 spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala diff --git a/.github/workflows/pr_build_linux.yml b/.github/workflows/pr_build_linux.yml index 192dc8ffb32..5650d2985f3 100644 --- a/.github/workflows/pr_build_linux.yml +++ b/.github/workflows/pr_build_linux.yml @@ -478,6 +478,7 @@ jobs: org.apache.comet.iceberg.IcebergReflectionSuite org.apache.comet.serde.operator.IcebergWriteProtoTranslationSuite org.apache.comet.csv.CometCsvNativeReadSuite + org.apache.comet.text.CometTextNativeReadSuite org.apache.comet.CometFuzzTestSuite org.apache.comet.CometFuzzIcebergSuite org.apache.comet.DataGeneratorSuite diff --git a/.github/workflows/pr_build_macos.yml b/.github/workflows/pr_build_macos.yml index 0729cb26f67..1867e669294 100644 --- a/.github/workflows/pr_build_macos.yml +++ b/.github/workflows/pr_build_macos.yml @@ -182,6 +182,7 @@ jobs: org.apache.comet.iceberg.IcebergReflectionSuite org.apache.comet.serde.operator.IcebergWriteProtoTranslationSuite org.apache.comet.csv.CometCsvNativeReadSuite + org.apache.comet.text.CometTextNativeReadSuite org.apache.comet.CometFuzzTestSuite org.apache.comet.CometFuzzIcebergSuite org.apache.comet.DataGeneratorSuite diff --git a/docs/source/user-guide/latest/datasources.md b/docs/source/user-guide/latest/datasources.md index 458538a102d..6bc58d331b4 100644 --- a/docs/source/user-guide/latest/datasources.md +++ b/docs/source/user-guide/latest/datasources.md @@ -51,6 +51,17 @@ converted into Arrow format, allowing the Comet pipeline to take over after that Comet does not provide a Rust-based JSON scan, but when `spark.comet.convert.json.enabled` is enabled, data is immediately converted into Arrow format, allowing the Comet pipeline to take over after that. +### Text + +Comet provides experimental Rust-based text scan support (a single `value: string` column). When +`spark.comet.scan.text.v2.enabled` is enabled, text files are read in Rust. This feature is experimental and +performance benefits are workload-dependent. Only Spark's DataSource V2 text scan is accelerated, and Spark reads +text through the V1 API by default, so also remove `text` from `spark.sql.sources.useV1SourceList`. The native scan +falls back to Spark for cases it does not handle -- including partitioned tables, compressed files, files large +enough that Spark splits them into byte ranges, `wholetext` files larger than roughly 2GB (capped at +`min(spark.sql.files.maxPartitionBytes, 2GB)`), unsupported filesystem schemes, and `input_file_name()` or +`ignoreCorruptFiles`/`ignoreMissingFiles` usage. It targets small lookup tables such as a broadcast join's build side. + ### Other Spark inputs Comet can also convert the output of these Spark inputs to Arrow format, so that the operators diff --git a/native/core/src/execution/jni_api.rs b/native/core/src/execution/jni_api.rs index f8e9e497f6f..2538e22b171 100644 --- a/native/core/src/execution/jni_api.rs +++ b/native/core/src/execution/jni_api.rs @@ -488,6 +488,7 @@ fn op_name(op: &OpStruct) -> &'static str { OpStruct::ParquetWriter(_) => "ParquetWriter", OpStruct::Explode(_) => "Explode", OpStruct::CsvScan(_) => "CsvScan", + OpStruct::TextScan(_) => "TextScan", OpStruct::ShuffleScan(_) => "ShuffleScan", OpStruct::BroadcastNestedLoopJoin(_) => "BroadcastNestedLoopJoin", OpStruct::Sample(_) => "Sample", diff --git a/native/core/src/execution/operators/mod.rs b/native/core/src/execution/operators/mod.rs index a15a528f90b..fb6a81ec2c2 100644 --- a/native/core/src/execution/operators/mod.rs +++ b/native/core/src/execution/operators/mod.rs @@ -41,5 +41,7 @@ mod csv_scan; pub mod projection; mod scan; mod shuffle_scan; +mod text_scan; pub use csv_scan::init_csv_datasource_exec; pub use shuffle_scan::ShuffleScanExec; +pub use text_scan::init_text_datasource_exec; diff --git a/native/core/src/execution/operators/text_scan.rs b/native/core/src/execution/operators/text_scan.rs new file mode 100644 index 00000000000..743d63e0dc6 --- /dev/null +++ b/native/core/src/execution/operators/text_scan.rs @@ -0,0 +1,614 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Native Text file scan, mirroring Spark's `text` data source. +//! +//! Spark's text reader produces a single `value: string` column. With the default options each +//! line of the file is one row (split on `\n`, `\r\n`, or `\r`, with the terminator stripped). +//! With `wholetext=true` the whole file is a single row. A custom `lineSep` splits on exactly that +//! byte sequence instead of the universal-newline set. See the `TextOptions`/`HadoopFileLinesReader` +//! logic in Spark's `org.apache.spark.sql.execution.datasources.text` package. + +use crate::execution::operators::ExecutionError; +use arrow::array::StringBuilder; +use arrow::datatypes::SchemaRef; +use arrow::record_batch::{RecordBatch, RecordBatchOptions}; +use datafusion::common::Result; +use datafusion::datasource::object_store::ObjectStoreUrl; +use datafusion::physical_expr::projection::ProjectionExprs; +use datafusion::physical_plan::metrics::{BaselineMetrics, ExecutionPlanMetricsSet}; +use datafusion::physical_plan::DisplayFormatType; +use datafusion_comet_common::decode_utf8_spark_lossy; +use datafusion_comet_proto::spark_operator::TextOptions; +use datafusion_datasource::file::FileSource; +use datafusion_datasource::file_compression_type::FileCompressionType; +use datafusion_datasource::file_groups::FileGroup; +use datafusion_datasource::file_scan_config::{FileScanConfig, FileScanConfigBuilder}; +use datafusion_datasource::file_stream::{FileOpenFuture, FileOpener}; +use datafusion_datasource::projection::{ProjectionOpener, SplitProjection}; +use datafusion_datasource::source::DataSourceExec; +use datafusion_datasource::{as_file_source, PartitionedFile, TableSchema}; +use futures::StreamExt; +use object_store::{ObjectStore, ObjectStoreExt}; +use std::borrow::Cow; +use std::fmt; +use std::io::Read; +use std::sync::Arc; + +/// Batch size used when the execution framework does not set one before opening the file. +const DEFAULT_TEXT_BATCH_SIZE: usize = 8192; + +/// UTF-8 byte-order mark. Spark's line reader strips a leading one; wholetext keeps it. +const UTF8_BOM: &[u8] = &[0xEF, 0xBB, 0xBF]; + +/// Cap on a batch's decoded `value` buffer bytes, kept well below Arrow's 2GB 32-bit offset limit +/// so a batch of many rows (or a few long lines) cannot overflow. Measured against the decoded +/// string length, since a lossy UTF-8 decode can expand ill-formed bytes up to 3x. +const MAX_BATCH_VALUE_BYTES: usize = 1 << 30; + +pub fn init_text_datasource_exec( + object_store_url: ObjectStoreUrl, + file_groups: Vec>, + data_schema: SchemaRef, + _partition_schema: SchemaRef, + projection_vector: Vec, + text_options: &TextOptions, +) -> Result, ExecutionError> { + let text_source = TextSource::new(data_schema, text_options); + + let file_groups = file_groups.into_iter().map(FileGroup::new).collect(); + + let file_scan_config = FileScanConfigBuilder::new(object_store_url, Arc::new(text_source)) + .with_file_groups(file_groups) + .with_projection_indices(Some(projection_vector))? + .build(); + + Ok(DataSourceExec::from_data_source(file_scan_config)) +} + +/// A [`FileSource`] for Spark's text format. The file schema is always the single `value` column; +/// projection (and partition columns, if any) is handled by [`ProjectionOpener`], exactly as the +/// CSV source does. +#[derive(Debug, Clone)] +struct TextSource { + table_schema: TableSchema, + projection: SplitProjection, + batch_size: Option, + metrics: ExecutionPlanMetricsSet, + whole_text: bool, + // The raw separator bytes when a custom `lineSep` is set; None means universal newline + // (`\n`, `\r\n`, `\r`). + line_sep: Option>, +} + +impl TextSource { + fn new(file_schema: SchemaRef, options: &TextOptions) -> Self { + let table_schema: TableSchema = file_schema.into(); + // `line_sep` arrives as the exact separator bytes Spark computed (`lineSeparatorInRead`, + // already encoded with the file's charset), so we split on them directly. + let line_sep = options.line_sep.clone().filter(|sep| !sep.is_empty()); + Self { + projection: SplitProjection::unprojected(&table_schema), + table_schema, + batch_size: None, + metrics: ExecutionPlanMetricsSet::new(), + whole_text: options.whole_text, + line_sep, + } + } +} + +impl From for Arc { + fn from(source: TextSource) -> Self { + as_file_source(source) + } +} + +impl FileSource for TextSource { + fn create_file_opener( + &self, + object_store: Arc, + base_config: &FileScanConfig, + partition_index: usize, + ) -> Result> { + // The inner opener must emit batches whose schema is the file schema projected by + // `file_indices` (that is what ProjectionOpener feeds its projector). For text that is + // either the single `value` column or, for a projection-less count, an empty schema. + let file_schema = self.table_schema.file_schema(); + let projected_file_schema = Arc::new(file_schema.project(&self.projection.file_indices)?); + let opener = Arc::new(TextOpener { + projected_file_schema, + file_compression_type: base_config.file_compression_type, + object_store, + partition_index, + batch_size: self.batch_size.unwrap_or(DEFAULT_TEXT_BATCH_SIZE), + metrics: self.metrics.clone(), + whole_text: self.whole_text, + line_sep: self.line_sep.clone(), + }) as Arc; + ProjectionOpener::try_new(self.projection.clone(), opener, file_schema) + } + + fn table_schema(&self) -> &TableSchema { + &self.table_schema + } + + fn with_batch_size(&self, batch_size: usize) -> Arc { + let mut conf = self.clone(); + conf.batch_size = Some(batch_size); + Arc::new(conf) + } + + fn try_pushdown_projection( + &self, + projection: &ProjectionExprs, + ) -> Result>> { + let mut source = self.clone(); + let new_projection = self.projection.source.try_merge(projection)?; + source.projection = SplitProjection::new(self.table_schema.file_schema(), &new_projection); + Ok(Some(Arc::new(source))) + } + + fn projection(&self) -> Option<&ProjectionExprs> { + Some(&self.projection.source) + } + + fn metrics(&self) -> &ExecutionPlanMetricsSet { + &self.metrics + } + + fn file_type(&self) -> &str { + "text" + } + + // A custom line separator can straddle a byte-range boundary, and we read whole files rather + // than boundary-aligned ranges, so this source must not let DataFusion repartition files by + // byte range. + fn supports_repartitioning(&self) -> bool { + false + } + + fn fmt_extra(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result { + match t { + DisplayFormatType::Default | DisplayFormatType::Verbose => { + write!(f, ", whole_text={}", self.whole_text) + } + DisplayFormatType::TreeRender => Ok(()), + } + } + + fn apply_expressions( + &self, + f: &mut dyn FnMut( + &Arc, + ) -> Result, + ) -> Result { + datafusion::physical_plan::apply_expression_roots(self.projection.source.iter(), f) + } +} + +/// A [`FileOpener`] that reads a whole text file and yields the projected file schema in batches. +struct TextOpener { + // The file schema projected by `file_indices`: the single `value` column, or an empty schema + // for a projection-less count. + projected_file_schema: SchemaRef, + file_compression_type: FileCompressionType, + object_store: Arc, + partition_index: usize, + batch_size: usize, + metrics: ExecutionPlanMetricsSet, + whole_text: bool, + line_sep: Option>, +} + +impl FileOpener for TextOpener { + fn open(&self, partitioned_file: PartitionedFile) -> Result { + let store = Arc::clone(&self.object_store); + let projected_file_schema = Arc::clone(&self.projected_file_schema); + let file_compression_type = self.file_compression_type; + let batch_size = self.batch_size; + let whole_text = self.whole_text; + let line_sep = self.line_sep.clone(); + let baseline_metrics = BaselineMetrics::new(&self.metrics, self.partition_index); + + Ok(Box::pin(async move { + let range = partitioned_file.range.clone(); + let location = partitioned_file.object_meta.location; + let raw = store.get(&location).await?.bytes().await?; + + // Decompress only when the file is actually compressed. On Comet's plan config the file + // is always UNCOMPRESSED (CometScanRule falls back for compressed files), so we borrow + // the fetched bytes directly and avoid a full-file copy; the decompress arm is a + // defensive handler should compression ever be plumbed through. A byte range never + // coexists with decompression (compressed files are non-splittable). + // + // NOTE: the whole object is read into memory. CometScanRule restricts native text to + // unsplit files (it falls back when Spark splits a file into byte ranges), so in + // practice this reads each small file once; the range filter below is a correctness + // safety net for any split that still reaches here. + let buf: Cow<[u8]> = if file_compression_type.is_compressed() { + let mut decoder = file_compression_type.convert_read(std::io::Cursor::new(raw))?; + let mut decoded = Vec::new(); + decoder.read_to_end(&mut decoded)?; + Cow::Owned(decoded) + } else { + Cow::Borrowed(raw.as_ref()) + }; + + let mut timer = baseline_metrics.elapsed_compute().timer(); + // Spark's line reader (Hadoop LineRecordReader.skipUtfByteOrderMark) strips a leading + // UTF-8 BOM at the start of the file. Mirror that in line mode for the split that starts + // at file offset 0 (native only reads unsplit files, so start is always 0; a custom + // lineSep still strips, matching Hadoop). wholetext keeps the BOM, as Spark's + // WholeTextFileRecordReader does. + let at_file_start = range.as_ref().map(|r| r.start == 0).unwrap_or(true); + let content = strip_leading_bom(&buf, whole_text, at_file_start); + let mut lines = split_lines(content, whole_text, line_sep.as_deref()); + // Spark may split an uncompressed file into byte ranges (TextScan.isSplitable). A line + // belongs to the split whose [start, end) contains the line's start offset -- so each + // line is emitted by exactly one split, with no duplication or gaps across splits. See + // DataFusion's CsvOpener for the same rule. wholetext files are never split. + if !whole_text { + if let Some(range) = range { + lines = filter_lines_by_range(content, lines, range.start, range.end); + } + } + let batches = build_batches(&projected_file_schema, lines, batch_size)?; + timer.stop(); + + Ok(futures::stream::iter(batches.into_iter().map(Ok)).boxed()) + })) + } +} + +/// Strip a single leading UTF-8 BOM, matching Hadoop LineRecordReader.skipUtfByteOrderMark: only in +/// line mode (not wholetext) and only for the split that starts at file offset 0. +fn strip_leading_bom(buf: &[u8], whole_text: bool, at_file_start: bool) -> &[u8] { + if !whole_text && at_file_start && buf.starts_with(UTF8_BOM) { + &buf[UTF8_BOM.len()..] + } else { + buf + } +} + +/// Split a decompressed file buffer into text lines following Spark's semantics. +fn split_lines<'a>(buf: &'a [u8], whole_text: bool, line_sep: Option<&[u8]>) -> Vec<&'a [u8]> { + if whole_text { + // One row per file; the whole content (including internal separators) is a single value. + return vec![buf]; + } + match line_sep { + Some(sep) => split_on_separator(buf, sep), + None => split_universal(buf), + } +} + +/// Universal-newline splitting: a line ends at `\n`, `\r\n`, or `\r`, and the terminator is +/// stripped. A trailing terminator does not produce a final empty line (Hadoop LineRecordReader). +fn split_universal(buf: &[u8]) -> Vec<&[u8]> { + let mut lines = Vec::new(); + let mut start = 0; + let mut i = 0; + while i < buf.len() { + match buf[i] { + b'\n' => { + lines.push(&buf[start..i]); + i += 1; + start = i; + } + b'\r' => { + lines.push(&buf[start..i]); + i += 1; + if i < buf.len() && buf[i] == b'\n' { + i += 1; + } + start = i; + } + _ => i += 1, + } + } + if start < buf.len() { + lines.push(&buf[start..]); + } + lines +} + +/// Split on an exact separator byte sequence; the trailing separator does not produce an empty +/// final line. +fn split_on_separator<'a>(buf: &'a [u8], sep: &[u8]) -> Vec<&'a [u8]> { + let mut lines = Vec::new(); + let mut start = 0; + let mut i = 0; + while i + sep.len() <= buf.len() { + if &buf[i..i + sep.len()] == sep { + lines.push(&buf[start..i]); + i += sep.len(); + start = i; + } else { + i += 1; + } + } + if start < buf.len() { + lines.push(&buf[start..]); + } + lines +} + +/// Keep only the lines whose start offset falls in the split's byte range `[start, end)`. +/// `lines` must be sub-slices of `buf` (they are, as produced by the split helpers), so a line's +/// start offset is its pointer distance from the buffer start. The split helpers emit lines in +/// strictly increasing start-offset order, so the qualifying lines are a contiguous run; find its +/// bounds by binary search and keep that slice in place without a second allocation. +fn filter_lines_by_range<'a>( + buf: &[u8], + mut lines: Vec<&'a [u8]>, + start: i64, + end: i64, +) -> Vec<&'a [u8]> { + let base = buf.as_ptr() as usize; + let start = start.max(0) as usize; + let end = end.max(0) as usize; + let hi = lines.partition_point(|line| (line.as_ptr() as usize - base) < end); + let lo = lines + .partition_point(|line| (line.as_ptr() as usize - base) < start) + .min(hi); + lines.truncate(hi); + lines.drain(..lo); + lines +} + +/// Build record batches from raw line bytes. When `projected_schema` has the `value` column, each +/// line becomes a string; when it is empty (a projection-less count), only the row count is kept. +/// +/// Arrow strings must be valid UTF-8, so ill-formed bytes are replaced with U+FFFD via +/// `decode_utf8_spark_lossy`. Spark's text reader instead stores the raw line bytes and only +/// substitutes on decode, so a plain read + collect matches, but byte-level operations over +/// invalid UTF-8 (octet_length, cast to binary, byte-exact predicates) can differ. This is an +/// inherent limitation of representing text bytes as an Arrow string. +fn build_batches( + projected_schema: &SchemaRef, + lines: Vec<&[u8]>, + batch_size: usize, +) -> Result> { + let emit_value = !projected_schema.fields().is_empty(); + let batch_rows = batch_size.max(1); + let mut batches = Vec::new(); + let mut i = 0; + while i < lines.len() { + let batch = if emit_value { + // Decode each line once (decode_utf8_spark_lossy borrows for valid UTF-8, allocates only + // for ill-formed input) and accumulate against the DECODED size, since a lossy decode can + // expand ill-formed bytes up to 3x. Start a new batch before the value buffer would reach + // Arrow's 32-bit offset limit. Always take at least one line so progress is guaranteed; + // if a single decoded value alone would overflow, fail cleanly rather than panic in Arrow. + let mut decoded: Vec> = Vec::new(); + let mut data_bytes = 0usize; + while i < lines.len() && decoded.len() < batch_rows { + let value = decode_utf8_spark_lossy(lines[i]); + let len = value.len(); + if len > i32::MAX as usize { + return Err(datafusion::common::DataFusionError::Execution(format!( + "Text value of {len} bytes exceeds Arrow's 2GB string-offset limit" + ))); + } + if !decoded.is_empty() && data_bytes + len > MAX_BATCH_VALUE_BYTES { + break; + } + data_bytes += len; + decoded.push(value); + i += 1; + } + let mut builder = StringBuilder::with_capacity(decoded.len(), data_bytes); + for value in &decoded { + builder.append_value(value); + } + RecordBatch::try_new( + Arc::clone(projected_schema), + vec![Arc::new(builder.finish())], + )? + } else { + let take = (lines.len() - i).min(batch_rows); + let options = RecordBatchOptions::new().with_row_count(Some(take)); + i += take; + RecordBatch::try_new_with_options(Arc::clone(projected_schema), vec![], &options)? + }; + batches.push(batch); + } + Ok(batches) +} + +#[cfg(test)] +mod tests { + use super::*; + use arrow::array::{Array, StringArray}; + use arrow::datatypes::{DataType, Field, Schema}; + + fn lines_as_str(buf: &[u8], whole_text: bool, line_sep: Option<&[u8]>) -> Vec { + split_lines(buf, whole_text, line_sep) + .iter() + .map(|l| String::from_utf8_lossy(l).into_owned()) + .collect() + } + + #[test] + fn universal_newline_variants() { + // \n, \r\n, and \r all split; the terminator is stripped. + assert_eq!( + lines_as_str(b"a\nb\r\nc\rd", false, None), + vec!["a", "b", "c", "d"] + ); + } + + #[test] + fn trailing_terminator_no_empty_line() { + assert_eq!(lines_as_str(b"a\nb\n", false, None), vec!["a", "b"]); + assert_eq!(lines_as_str(b"a\r\n", false, None), vec!["a"]); + assert_eq!(lines_as_str(b"a\r", false, None), vec!["a"]); + } + + #[test] + fn empty_interior_lines_are_kept() { + assert_eq!(lines_as_str(b"a\n\nb", false, None), vec!["a", "", "b"]); + // A lone newline is one empty record. + assert_eq!(lines_as_str(b"\n", false, None), vec![""]); + } + + #[test] + fn empty_buffer_has_no_lines() { + assert!(lines_as_str(b"", false, None).is_empty()); + } + + #[test] + fn no_terminator_is_single_line() { + assert_eq!( + lines_as_str(b"only one line", false, None), + vec!["only one line"] + ); + } + + #[test] + fn custom_single_char_separator() { + // A comma and a bare newline are literal content when lineSep is '|'. + assert_eq!( + lines_as_str(b"a,b|c\nd|e", false, Some(b"|")), + vec!["a,b", "c\nd", "e"] + ); + // Trailing separator does not add an empty line. + assert_eq!(lines_as_str(b"a|b|", false, Some(b"|")), vec!["a", "b"]); + } + + #[test] + fn custom_multi_byte_separator() { + assert_eq!( + lines_as_str(b"a<>b<>c", false, Some(b"<>")), + vec!["a", "b", "c"] + ); + } + + #[test] + fn whole_text_is_one_row_with_separators_preserved() { + assert_eq!(lines_as_str(b"a\nb\nc", true, None), vec!["a\nb\nc"]); + } + + #[test] + fn non_ascii_is_preserved() { + assert_eq!( + lines_as_str("café\n日本語".as_bytes(), false, None), + vec!["café", "日本語"] + ); + } + + #[test] + fn build_batches_value_column() { + let schema = Arc::new(Schema::new(vec![Field::new("value", DataType::Utf8, true)])); + let lines: Vec<&[u8]> = vec![b"x", b"y", b"z"]; + let batches = build_batches(&schema, lines, 2).unwrap(); + // batch_size 2 -> two batches (2 rows, 1 row). + assert_eq!(batches.len(), 2); + assert_eq!(batches.iter().map(|b| b.num_rows()).sum::(), 3); + let col = batches[0] + .column(0) + .as_any() + .downcast_ref::() + .unwrap(); + assert_eq!(col.value(0), "x"); + assert_eq!(col.value(1), "y"); + } + + #[test] + fn build_batches_empty_projection_keeps_row_count() { + // The count(*) case: an empty projected schema, rows preserved with no columns. + let schema = Arc::new(Schema::empty()); + let lines: Vec<&[u8]> = vec![b"x", b"y", b"z"]; + let batches = build_batches(&schema, lines, 8192).unwrap(); + assert_eq!(batches.len(), 1); + assert_eq!(batches[0].num_columns(), 0); + assert_eq!(batches[0].num_rows(), 3); + } + + #[test] + fn strips_leading_utf8_bom_in_line_mode() { + let bom = [0xEFu8, 0xBB, 0xBF]; + let with_hello = [&bom[..], b"hello"].concat(); + // Line mode at file start: the BOM is stripped. + assert_eq!(strip_leading_bom(&with_hello, false, true), b"hello"); + // BOM then newline -> ["", "hello"] after split. + let bom_nl = [&bom[..], b"\nhello"].concat(); + assert_eq!( + lines_as_str(strip_leading_bom(&bom_nl, false, true), false, None), + vec!["", "hello"] + ); + // BOM-only file -> empty after strip -> zero lines. + assert!(split_lines(strip_leading_bom(&bom, false, true), false, None).is_empty()); + // Not stripped: wholetext mode, a non-start split, or no BOM present. + assert_eq!(strip_leading_bom(&with_hello, true, true), &with_hello[..]); + assert_eq!( + strip_leading_bom(&with_hello, false, false), + &with_hello[..] + ); + assert_eq!(strip_leading_bom(b"hello", false, true), b"hello"); + } + + #[test] + fn build_batches_replaces_invalid_utf8() { + // Ill-formed bytes become U+FFFD (matches Spark's new String(bytes, UTF_8) on collect). + let schema = Arc::new(Schema::new(vec![Field::new("value", DataType::Utf8, true)])); + let bad: &[u8] = &[b'a', 0xff, b'b']; + let lines: Vec<&[u8]> = vec![bad]; + let batches = build_batches(&schema, lines, 8192).unwrap(); + assert_eq!(batches.len(), 1); + let col = batches[0] + .column(0) + .as_any() + .downcast_ref::() + .unwrap(); + assert_eq!(col.value(0), "a\u{FFFD}b"); + } + + #[test] + fn range_split_partitions_lines_without_duplication() { + // Two contiguous splits over one file must together yield every line exactly once. + let buf = b"aaa\nbbb\nccc\nddd"; // offsets: aaa=0 bbb=4 ccc=8 ddd=12 + let lines = split_universal(buf); + let first = filter_lines_by_range(buf, lines.clone(), 0, 6); + let second = filter_lines_by_range(buf, lines.clone(), 6, 14); + let as_str = |v: Vec<&[u8]>| { + v.iter() + .map(|l| String::from_utf8_lossy(l).into_owned()) + .collect::>() + }; + assert_eq!(as_str(first), vec!["aaa", "bbb"]); + assert_eq!(as_str(second), vec!["ccc", "ddd"]); + } + + #[test] + fn range_split_line_straddling_boundary_belongs_to_first_split() { + // A line whose start offset is in the first split but that extends past the split end is + // read fully by the first split and NOT repeated by the next. + let buf = b"abcdef\nghij"; // abcdef=0, ghij=7 + let lines = split_universal(buf); + let first = filter_lines_by_range(buf, lines.clone(), 0, 3); + let second = filter_lines_by_range(buf, lines.clone(), 3, 11); + let as_str = |v: Vec<&[u8]>| { + v.iter() + .map(|l| String::from_utf8_lossy(l).into_owned()) + .collect::>() + }; + assert_eq!(as_str(first), vec!["abcdef"]); + assert_eq!(as_str(second), vec!["ghij"]); + } +} diff --git a/native/core/src/execution/planner.rs b/native/core/src/execution/planner.rs index ceca9e657af..254560a700c 100644 --- a/native/core/src/execution/planner.rs +++ b/native/core/src/execution/planner.rs @@ -35,6 +35,7 @@ mod delta_scan; mod lance_scan; use crate::execution::operators::init_csv_datasource_exec; +use crate::execution::operators::init_text_datasource_exec; use crate::execution::operators::DynamicFilterJoinExec; use crate::execution::operators::IcebergScanExec; use crate::execution::operators::TopKReaderFilterExec; @@ -2116,6 +2117,46 @@ impl PhysicalPlanner { Arc::new(SparkPlan::new(spark_plan.plan_id, scan, vec![])), )) } + OpStruct::TextScan(scan) => { + let data_schema = convert_spark_types_to_arrow_schema(scan.data_schema.as_slice()); + let partition_schema = + convert_spark_types_to_arrow_schema(scan.partition_schema.as_slice()); + let projection_vector: Vec = + scan.projection_vector.iter().map(|i| *i as usize).collect(); + let object_store_options: HashMap = + scan.object_store_options.clone(); + let one_file = scan + .file_partitions + .first() + .and_then(|f| f.partitioned_file.first()) + .map(|f| f.file_path.clone()) + .ok_or(GeneralError("Failed to locate file".to_string()))?; + let (object_store_url, _, _) = prepare_object_store_with_configs( + self.session_ctx.runtime_env(), + one_file, + &object_store_options, + )?; + let files = self.get_partitioned_files( + &scan.file_partitions[self.partition as usize], + &object_store_options, + )?; + let file_groups: Vec> = vec![files]; + let scan = init_text_datasource_exec( + object_store_url, + file_groups, + data_schema, + partition_schema, + projection_vector, + scan.text_options + .as_ref() + .ok_or_else(|| GeneralError("TextScan missing text_options".to_string()))?, + )?; + Ok(( + vec![], + vec![], + Arc::new(SparkPlan::new(spark_plan.plan_id, scan, vec![])), + )) + } OpStruct::Scan(scan) => { let data_types = scan.fields.iter().map(to_arrow_datatype).collect_vec(); diff --git a/native/core/src/execution/planner/operator_registry.rs b/native/core/src/execution/planner/operator_registry.rs index 70ed309ebeb..47c4c6fb1f9 100644 --- a/native/core/src/execution/planner/operator_registry.rs +++ b/native/core/src/execution/planner/operator_registry.rs @@ -60,6 +60,7 @@ pub enum OperatorType { HashJoin, Window, CsvScan, + TextScan, } /// Global registry of operator builders @@ -175,6 +176,7 @@ fn get_operator_type(spark_operator: &Operator) -> Option { OpStruct::Window(_) => Some(OperatorType::Window), OpStruct::Explode(_) => None, // Not yet in OperatorType enum OpStruct::CsvScan(_) => Some(OperatorType::CsvScan), + OpStruct::TextScan(_) => Some(OperatorType::TextScan), OpStruct::ShuffleScan(_) => Some(OperatorType::ShuffleScan), OpStruct::BroadcastNestedLoopJoin(_) => None, OpStruct::RangeScan(_) => None, diff --git a/native/proto/src/proto/operator.proto b/native/proto/src/proto/operator.proto index 97c65f0da36..9d4c0633b71 100644 --- a/native/proto/src/proto/operator.proto +++ b/native/proto/src/proto/operator.proto @@ -70,6 +70,7 @@ message Operator { IcebergWrite iceberg_write = 120; RangeScan range_scan = 121; MergeRows merge_rows = 122; + TextScan text_scan = 123; // Extension point for optional, out-of-tree contrib scans (Delta, Lance, ...). The concrete // scan message (e.g. `DeltaScan`) is packed into this envelope on the JVM side and dispatched // by `type_url` on the native side. Using a single permanent field -- rather than a new oneof @@ -220,6 +221,24 @@ message CsvOptions { bool truncated_rows = 8; } +// Native Text scan, mirroring Spark's `text` data source (a single `value: string` column). +message TextScan { + repeated SparkStructField data_schema = 1; + repeated SparkStructField partition_schema = 2; + repeated int32 projection_vector = 3; + repeated SparkFilePartition file_partitions = 4; + map object_store_options = 5; + TextOptions text_options = 6; +} + +message TextOptions { + // When true, each file is read as a single row instead of being split into lines. + bool whole_text = 1; + // Custom line separator, as the exact bytes Spark computed (lineSeparatorInRead, already encoded + // with the file's charset). When unset, lines are split on `\n`, `\r\n`, or `\r`. + optional bytes line_sep = 2; +} + // A typed Iceberg primitive value, shared by partition values and predicate literals. The oneof // case is the type tag (iceberg-rust's Datum is self-typed and Predicate::bind does not coerce, so // the type must be known at construction). Grows by adding a case as Iceberg adds primitives diff --git a/spark/src/main/scala/org/apache/comet/CometConf.scala b/spark/src/main/scala/org/apache/comet/CometConf.scala index 9bd32816c64..cfec05d3301 100644 --- a/spark/src/main/scala/org/apache/comet/CometConf.scala +++ b/spark/src/main/scala/org/apache/comet/CometConf.scala @@ -171,6 +171,17 @@ object CometConf extends ShimCometConf { .booleanConf .createWithDefault(false) + val COMET_TEXT_V2_NATIVE_ENABLED: ConfigEntry[Boolean] = + conf("spark.comet.scan.text.v2.enabled") + .category(CATEGORY_TESTING) + .doc( + "Whether to use the native Comet V2 reader for the text file format (a single " + + "'value: string' column). Requires the V2 text source (remove 'text' from " + + "spark.sql.sources.useV1SourceList). Default: false. " + + "Experimental: Performance benefits are workload-dependent.") + .booleanConf + .createWithDefault(false) + val COMET_PARQUET_ROW_FILTER_PUSHDOWN_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.parquet.rowFilterPushdown.enabled") .category(CATEGORY_PARQUET) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala index 32faea60e65..2819191a069 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala @@ -48,6 +48,7 @@ import org.apache.spark.sql.execution.datasources.v2.{BatchScanExec, V2CommandEx import org.apache.spark.sql.execution.datasources.v2.csv.CSVScan import org.apache.spark.sql.execution.datasources.v2.json.JsonScan import org.apache.spark.sql.execution.datasources.v2.parquet.ParquetScan +import org.apache.spark.sql.execution.datasources.v2.text.TextScan import org.apache.spark.sql.execution.exchange.{BroadcastExchangeExec, BroadcastExchangeLike, Exchange, ReusedExchangeExec, ShuffleExchangeExec, ShuffleExchangeLike} import org.apache.spark.sql.execution.joins.{BroadcastHashJoinExec, BroadcastNestedLoopJoinExec, ShuffledHashJoinExec, SortMergeJoinExec} import org.apache.spark.sql.execution.window.WindowExec @@ -444,6 +445,9 @@ case class CometExecRule(session: SparkSession, queryStagePrep: Boolean = false) case scan: CometBatchScanExec if scan.wrapped.scan.isInstanceOf[CSVScan] => convertToComet(scan, CometCsvNativeScanExec).getOrElse(scan.wrapped) + case scan: CometBatchScanExec if scan.wrapped.scan.isInstanceOf[TextScan] => + convertToComet(scan, CometTextNativeScanExec).getOrElse(scan) + // Comet JVM + native scan for V1 and V2 case op if isCometScan(op) => convertToComet(op, CometScanWrapper).getOrElse(op) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index edda7d3fabd..65d55d6b2c4 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -38,9 +38,10 @@ import org.apache.spark.sql.catalyst.util.{sideBySide, ArrayBasedMapData, DateTi import org.apache.spark.sql.catalyst.util.ResolveDefaultColumns.getExistenceDefaultValues import org.apache.spark.sql.comet.{CometBatchScanExec, CometScanExec} import org.apache.spark.sql.execution.{FileSourceScanExec, InSubqueryExec, SparkPlan, SubqueryAdaptiveBroadcastExec} -import org.apache.spark.sql.execution.datasources.HadoopFsRelation +import org.apache.spark.sql.execution.datasources.{FilePartition, HadoopFsRelation} import org.apache.spark.sql.execution.datasources.v2.BatchScanExec import org.apache.spark.sql.execution.datasources.v2.csv.CSVScan +import org.apache.spark.sql.execution.datasources.v2.text.TextScan import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ @@ -468,6 +469,147 @@ case class CometScanRule(session: SparkSession) withFallbackReasons(scanExec, fallbackReasons.toSet) } + case scan: TextScan if COMET_TEXT_V2_NATIVE_ENABLED.get() => + if (scanExec.output.exists(_.isMetadataCol)) { + return withFallbackReason( + scanExec, + "Metadata columns are not supported for Text V2 scans") + } + // A native scan is useless without native execution: with exec disabled, CometExecRule + // leaves this CometBatchScanExec unconverted and it fails at runtime. Require native exec. + if (!COMET_EXEC_ENABLED.get()) { + return withFallbackReason( + scanExec, + s"Native Text scan requires ${COMET_EXEC_ENABLED.key} to be enabled") + } + // input_file_name/_block_start/_block_length read a thread-local that Spark's FileScanRDD + // sets; the native DataFusion scan does not, so they would return "" / -1. Same guard as + // the native Parquet path. + if (plan.exists(node => + node.expressions.exists(_.exists { + case _: InputFileName | _: InputFileBlockStart | _: InputFileBlockLength => true + case _ => false + }))) { + return withFallbackReason( + scanExec, + "Native Text scan is not compatible with input_file_name, " + + "input_file_block_start, or input_file_block_length") + } + val fallbackReasons = new ListBuffer[String]() + val schemaSupported = + CometBatchScanExec.isSchemaSupported(scan.readDataSchema, fallbackReasons) + if (!schemaSupported) { + fallbackReasons += s"Schema ${scan.readDataSchema} is not supported" + } + // Spark's text source allows only a single data column (TextScan.verifyReadSchema). A + // user-forced multi-column read schema is invalid; fall back so Spark raises its structured + // AnalysisException rather than letting the native reader fail with an opaque error. + if (scan.readDataSchema.size > 1) { + fallbackReasons += "Text data source supports only a single column" + } + // Partition columns are not implemented for native Text scans (unlike the native CSV path, + // which forwards them). Fall back to Spark for partitioned reads rather than drop them. + if (scan.readPartitionSchema.nonEmpty) { + fallbackReasons += "Comet does not support partition columns in native Text scans" + } + // The native reader loads each file whole. Spark splits a large uncompressed text file + // into byte-range partitions (TextScan.isSplitable), and each split would then re-read and + // buffer the whole file with no memory-pool reservation. Restrict native execution to + // unsplit files (the small build-side lookups this targets); Spark handles large files. + val hasSplitFile = scanExec.inputPartitions.exists { + case fp: FilePartition => fp.files.exists(_.start != 0) + case _ => false + } + if (hasSplitFile) { + fallbackReasons += "Comet native Text scan does not support files split into byte " + + "ranges (large files)" + } + // Comet's native plan does not carry a compression codec, so the native reader would feed + // raw compressed bytes to the line splitter. Spark auto-detects a codec by file extension + // (e.g. .gz, .bz2). Fall back when any input file is compressed. Use the options-aware + // Hadoop conf so a codec registered via a per-read option is honored (as Spark does). + val hadoopConf = session.sessionState.newHadoopConfWithOptions(scan.options.asScala.toMap) + val codecFactory = new org.apache.hadoop.io.compress.CompressionCodecFactory(hadoopConf) + val hasCompressedFile = scan.fileIndex.inputFiles.exists { path => + codecFactory.getCodec(new org.apache.hadoop.fs.Path(path)) != null + } + if (hasCompressedFile) { + fallbackReasons += "Comet does not support compressed text files" + } + // Spark skips unreadable/missing files when these are set; the native reader's DataFusion + // FileStream fails the whole scan instead (OnError::Fail). Fall back to match Spark. + if (SQLConf.get.ignoreCorruptFiles || scan.options.getBoolean( + "ignoreCorruptFiles", + false)) { + fallbackReasons += "Comet native Text scan does not support ignoreCorruptFiles" + } + if (SQLConf.get.ignoreMissingFiles || scan.options.getBoolean( + "ignoreMissingFiles", + false)) { + fallbackReasons += "Comet native Text scan does not support ignoreMissingFiles" + } + // A wholetext file is never split (TextScan.isSplitable is false for wholeText), so the + // split-file guard above does not bound it. Reading a very large file whole is unbounded + // memory, and the whole file becomes one Arrow string value. Because a lossy UTF-8 decode + // can expand ill-formed bytes up to 3x, cap the raw file size at Int.MaxValue/3 so the + // decoded value stays under Arrow's 2GB 32-bit string-offset limit; fall back above that. + // (The native reader also fails cleanly rather than panicking if a value still overflows.) + val textOptions = + new org.apache.spark.sql.execution.datasources.text.TextOptions( + scan.options.asScala.toMap) + if (textOptions.wholeText) { + val maxWholeTextBytes = + math.min(session.sessionState.conf.filesMaxPartitionBytes, Int.MaxValue.toLong / 3) + val hasLargeWholeTextFile = scanExec.inputPartitions.exists { + case fp: FilePartition => fp.files.exists(_.length > maxWholeTextBytes) + case _ => false + } + if (hasLargeWholeTextFile) { + fallbackReasons += "Comet native Text scan does not support large wholetext files " + + s"(over ${SQLConf.FILES_MAX_PARTITION_BYTES.key}, capped below 2GB)" + } + } + // Comet's native reader opens files through object_store, which only understands a fixed + // set of URL schemes. A text file on a custom Hadoop scheme (viewfs://, oss://, ...) would + // be claimed here then hard-fail at execution, whereas Spark reads it via the Hadoop FS + // API. Decline such schemes (same gate as the native Parquet path); hdfs:// routes through + // libhdfs and is left to native. + val libhdfsSchemes: Set[String] = COMET_LIBHDFS_SCHEMES.get() match { + case Some(s) => NativeConfig.parseSchemeSet(s) + case None => Set("hdfs") + } + val s3CompliantSchemes = NativeConfig.resolveS3CompliantSchemes(hadoopConf) + val roots = CometScanRule.classifyRootPaths( + scan.fileIndex.rootPaths.map(_.toUri), + libhdfsSchemes, + s3CompliantSchemes) + val unsupportedFsSchemes = roots.iterator.flatMap { root => + root.scheme.filter(_ => + !root.isLibhdfs && !CometScanRule.isNativelyReadableScheme( + root.uri, + s3CompliantSchemes)) + }.toSet + if (unsupportedFsSchemes.nonEmpty) { + fallbackReasons += + s"Unsupported filesystem schemes: ${unsupportedFsSchemes.mkString(", ")}" + } + // A recognized scheme can still carry a path object_store rejects (e.g. a newline -> %0A), + // which native execution would hard-fail on. Root paths only. + val rejectedPath = roots.find(root => + root.scheme.isDefined && !root.isLibhdfs && !root.isAlias && + !CometScanRule.objectStoreAcceptsPath(root.uri)) + if (rejectedPath.nonEmpty) { + fallbackReasons += s"Native Text scan cannot open path '${rejectedPath.get.uri}': " + + "object_store rejects it (e.g. an unsupported character in the path)" + } + if (fallbackReasons.isEmpty) { + CometBatchScanExec( + scanExec.clone().asInstanceOf[BatchScanExec], + runtimeFilters = scanExec.runtimeFilters) + } else { + withFallbackReasons(scanExec, fallbackReasons.toSet) + } + // Iceberg scan - detected by class name. SparkStagedScan covers reads issued by // RewriteDataFiles (and similar maintenance actions) where the planner has already // staged FileScanTasks via ScanTaskSetManager. diff --git a/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala new file mode 100644 index 00000000000..3b46167e218 --- /dev/null +++ b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.spark.sql.comet + +import scala.jdk.CollectionConverters._ + +import org.apache.spark.sql.catalyst.expressions.{Attribute, SortOrder} +import org.apache.spark.sql.catalyst.plans.physical.{Partitioning, UnknownPartitioning} +import org.apache.spark.sql.execution.SparkPlan +import org.apache.spark.sql.execution.datasources.FilePartition +import org.apache.spark.sql.execution.datasources.text.TextOptions +import org.apache.spark.sql.execution.datasources.v2.BatchScanExec +import org.apache.spark.sql.execution.datasources.v2.text.TextScan + +import com.google.common.base.Objects + +import org.apache.comet.{CometConf, ConfigEntry} +import org.apache.comet.objectstore.NativeConfig +import org.apache.comet.serde.{CometOperatorSerde, OperatorOuterClass} +import org.apache.comet.serde.OperatorOuterClass.Operator +import org.apache.comet.serde.operator.{partition2Proto, schema2Proto} + +/* + * Native Text scan operator that delegates file reading to datafusion. Mirrors the native CSV + * scan path; produces a single `value: string` column. + */ +case class CometTextNativeScanExec( + override val nativeOp: Operator, + override val output: Seq[Attribute], + @transient override val originalPlan: BatchScanExec, + override val serializedPlanOpt: SerializedPlan) + extends CometLeafExec { + override val supportsColumnar: Boolean = true + + override val nodeName: String = "CometTextNativeScan" + + override def outputPartitioning: Partitioning = UnknownPartitioning( + originalPlan.inputPartitions.length) + + override def outputOrdering: Seq[SortOrder] = Nil + + override protected def doCanonicalize(): SparkPlan = { + CometTextNativeScanExec(nativeOp, output, originalPlan, serializedPlanOpt) + } + + override def equals(obj: Any): Boolean = { + obj match { + case other: CometTextNativeScanExec => + this.output == other.output && + this.serializedPlanOpt == other.serializedPlanOpt && + this.originalPlan == other.originalPlan + case _ => + false + } + } + + override def hashCode(): Int = { + Objects.hashCode(output, serializedPlanOpt, originalPlan) + } +} + +object CometTextNativeScanExec extends CometOperatorSerde[CometBatchScanExec] { + + override def enabledConfig: Option[ConfigEntry[Boolean]] = Some( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED) + + override def convert( + op: CometBatchScanExec, + builder: Operator.Builder, + childOp: Operator*): Option[Operator] = { + val textScanBuilder = OperatorOuterClass.TextScan.newBuilder() + val textScan = op.wrapped.scan.asInstanceOf[TextScan] + val sessionState = op.session.sessionState + val options = new TextOptions(textScan.options.asScala.toMap) + val filePartitions = op.inputPartitions.map(_.asInstanceOf[FilePartition]) + val textOptionsProto = textOptions2Proto(options) + val dataSchemaProto = schema2Proto(textScan.dataSchema) + val readSchemaFieldNames = textScan.readDataSchema.fieldNames + val projectionVector = textScan.dataSchema.zipWithIndex + .filter { case (field, _) => + readSchemaFieldNames.contains(field.name) + } + .map(_._2.asInstanceOf[Integer]) + val partitionSchemaProto = schema2Proto(textScan.readPartitionSchema) + val partitionsProto = filePartitions.map(partition2Proto(_, textScan.readPartitionSchema)) + + val objectStoreOptions = filePartitions.headOption + .flatMap { partitionFile => + val hadoopConf = sessionState + .newHadoopConfWithOptions(op.session.sparkContext.conf.getAll.toMap) + partitionFile.files.headOption + .map(file => NativeConfig.extractObjectStoreOptions(hadoopConf, file.pathUri)) + } + .getOrElse(Map.empty) + + textScanBuilder.putAllObjectStoreOptions(objectStoreOptions.asJava) + textScanBuilder.setTextOptions(textOptionsProto) + textScanBuilder.addAllFilePartitions(partitionsProto.asJava) + textScanBuilder.addAllDataSchema(dataSchemaProto.asJava) + textScanBuilder.addAllProjectionVector(projectionVector.asJava) + textScanBuilder.addAllPartitionSchema(partitionSchemaProto.asJava) + Some(builder.setTextScan(textScanBuilder).build()) + } + + override def createExec(nativeOp: Operator, op: CometBatchScanExec): CometNativeExec = { + CometTextNativeScanExec(nativeOp, op.output, op.wrapped, SerializedPlan(None)) + } + + private def textOptions2Proto(options: TextOptions): OperatorOuterClass.TextOptions = { + val textOptionsBuilder = OperatorOuterClass.TextOptions.newBuilder() + textOptionsBuilder.setWholeText(options.wholeText) + // Send the exact separator bytes Spark computed (already encoded with the file's charset via + // the `encoding` option); the native reader splits on these bytes directly. + options.lineSeparatorInRead.foreach(bytes => + textOptionsBuilder.setLineSep(com.google.protobuf.ByteString.copyFrom(bytes))) + textOptionsBuilder.build() + } +} diff --git a/spark/src/test/resources/test-data/text-test-1.txt b/spark/src/test/resources/test-data/text-test-1.txt new file mode 100644 index 00000000000..f10289c3616 --- /dev/null +++ b/spark/src/test/resources/test-data/text-test-1.txt @@ -0,0 +1,6 @@ +hello world +foo,bar,baz +"quoted stays literal" +café münchen naïve +日本語テスト +1234567890 diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala new file mode 100644 index 00000000000..e9d73aa1b0f --- /dev/null +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -0,0 +1,279 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet.text + +import java.io.File +import java.nio.charset.StandardCharsets +import java.nio.file.Files + +import org.apache.spark.sql.CometTestBase +import org.apache.spark.sql.functions.input_file_name +import org.apache.spark.sql.internal.SQLConf + +import org.apache.comet.CometConf + +class CometTextNativeReadSuite extends CometTestBase { + private val TEST_TEXT_PATH = "src/test/resources/test-data/text-test-1.txt" + + private def withTextConf(f: => Unit): Unit = { + withSQLConf( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "")(f) + } + + test("native text read - lines to value column") { + withTextConf { + val df = spark.read.text(TEST_TEXT_PATH) + checkSparkAnswerAndOperator(df) + } + } + + test("native text read - count") { + withTextConf { + val df = spark.read.text(TEST_TEXT_PATH).selectExpr("count(*) as c") + checkSparkAnswerAndOperator(df) + } + } + + test("native text read - filter and projection") { + withTextConf { + val df = spark.read.text(TEST_TEXT_PATH).where("value like '%bar%'") + checkSparkAnswerAndOperator(df) + } + } + + test("native text read - preserves line order within a file") { + withTempDir { dir => + val file = new File(dir, "ordered.txt") + val lines = (1 to 500).map(i => s"row-$i") + Files.write(file.toPath, lines.mkString("\n").getBytes(StandardCharsets.UTF_8)) + withTextConf { + val got = spark.read.text(file.getAbsolutePath).collect().map(_.getString(0)).toSeq + // checkSparkAnswer sorts both sides, so assert order explicitly here. + assert(got == lines, "native text scan must preserve within-file line order") + } + } + } + + test("native text read - fallback for input_file_name") { + withTextConf { + val df = spark.read.text(TEST_TEXT_PATH).withColumn("f", input_file_name()) + checkSparkAnswerAndFallbackReason( + df, + "Native Text scan is not compatible with input_file_name") + } + } + + test("native text read - fallback when native exec is disabled") { + withSQLConf( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + CometConf.COMET_EXEC_ENABLED.key -> "false", + SQLConf.USE_V1_SOURCE_LIST.key -> "") { + checkSparkAnswerAndFallbackReason( + spark.read.text(TEST_TEXT_PATH), + s"Native Text scan requires ${CometConf.COMET_EXEC_ENABLED.key} to be enabled") + } + } + + test("native text read - wholetext") { + withTextConf { + val df = spark.read.option("wholetext", "true").text(TEST_TEXT_PATH) + checkSparkAnswerAndOperator(df) + } + } + + test("native text read - custom lineSep") { + withTempDir { dir => + val file = new File(dir, "custom-linesep.txt") + Files.write(file.toPath, "a|b|café|日本".getBytes(StandardCharsets.UTF_8)) + withTextConf { + val df = spark.read.option("lineSep", "|").text(file.getAbsolutePath) + checkSparkAnswerAndOperator(df) + } + } + } + + test("native text read - custom lineSep with trailing separator") { + withTempDir { dir => + val file = new File(dir, "trailing-linesep.txt") + // A trailing separator must not produce an extra empty row (matches Spark, SPARK-23577). + Files.write(file.toPath, "a|b|c|".getBytes(StandardCharsets.UTF_8)) + withTextConf { + val df = spark.read.option("lineSep", "|").text(file.getAbsolutePath) + checkSparkAnswerAndOperator(df) + } + } + } + + test("native text read - non-ASCII multibyte custom lineSep") { + withTempDir { dir => + val file = new File(dir, "jp-linesep.txt") + // Japanese ideographic comma (U+3002, 3 UTF-8 bytes) as the record separator. + Files.write(file.toPath, "a。café。日本".getBytes(StandardCharsets.UTF_8)) + withTextConf { + val df = spark.read.option("lineSep", "。").text(file.getAbsolutePath) + checkSparkAnswerAndOperator(df) + } + } + } + + test("native text read - CRLF and CR line endings") { + withTempDir { dir => + val file = new File(dir, "crlf.txt") + // Mix of \r\n, \r, and \n; the default reader splits on all three. + Files.write(file.toPath, "a\r\nb\rc\nd".getBytes(StandardCharsets.UTF_8)) + withTextConf { + checkSparkAnswerAndOperator(spark.read.text(file.getAbsolutePath)) + } + } + } + + test("native text read - invalid UTF-8 bytes match Spark on collect") { + withTempDir { dir => + val file = new File(dir, "invalid-utf8.txt") + // 0xFF is not valid UTF-8; both readers render it as the replacement char on collect. + Files.write(file.toPath, Array[Byte]('a', 0xff.toByte, 'b', '\n', 'c')) + withTextConf { + checkSparkAnswerAndOperator(spark.read.text(file.getAbsolutePath)) + } + } + } + + test("native text read - wholetext count") { + withTextConf { + val df = spark.read + .option("wholetext", "true") + .text(TEST_TEXT_PATH) + .selectExpr("count(*) as c") + checkSparkAnswerAndOperator(df) + } + } + + test("native text read - falls back for files split into byte ranges") { + withTempDir { dir => + val file = new File(dir, "many-lines.txt") + val content = (1 to 2000).map(i => s"line-$i-日本テスト").mkString("\n") + Files.write(file.toPath, content.getBytes(StandardCharsets.UTF_8)) + withSQLConf( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "", + // Force the file to be split into several byte-range partitions. + SQLConf.FILES_MAX_PARTITION_BYTES.key -> "1024") { + val df = spark.read.text(file.getAbsolutePath) + assert(df.rdd.getNumPartitions > 1, "test requires the file to actually split") + checkSparkAnswerAndFallbackReason( + df, + "Comet native Text scan does not support files split into byte ranges") + } + } + } + + test("native text read - empty file") { + withTempDir { dir => + val file = new File(dir, "empty.txt") + Files.write(file.toPath, Array.emptyByteArray) + withTextConf { + checkSparkAnswerAndOperator(spark.read.text(file.getAbsolutePath)) + } + } + } + + test("native text read - directory of multiple small files") { + withTempDir { dir => + Files.write(new File(dir, "a.txt").toPath, "alpha\nbeta".getBytes(StandardCharsets.UTF_8)) + Files.write(new File(dir, "b.txt").toPath, "日本\ngamma".getBytes(StandardCharsets.UTF_8)) + Files.write(new File(dir, "c.txt").toPath, Array.emptyByteArray) + withTextConf { + checkSparkAnswerAndOperator(spark.read.text(dir.getAbsolutePath)) + } + } + } + + test("native text read - wholetext over a directory of files") { + withTempDir { dir => + Files.write( + new File(dir, "a.txt").toPath, + "line1\nline2\n".getBytes(StandardCharsets.UTF_8)) + Files.write(new File(dir, "b.txt").toPath, "".getBytes(StandardCharsets.UTF_8)) + withTextConf { + // One row per file, including one empty row for the empty file. + checkSparkAnswerAndOperator( + spark.read.option("wholetext", "true").text(dir.getAbsolutePath)) + } + } + } + + test("native text read - falls back for large wholetext files") { + withTempDir { dir => + val file = new File(dir, "big-wholetext.txt") + Files.write(file.toPath, ("x" * 100).getBytes(StandardCharsets.UTF_8)) + withSQLConf( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "", + // Tiny limit so the 100-byte wholetext file exceeds the size cap. + SQLConf.FILES_MAX_PARTITION_BYTES.key -> "16") { + val df = spark.read.option("wholetext", "true").text(file.getAbsolutePath) + checkSparkAnswerAndFallbackReason( + df, + "Comet native Text scan does not support large wholetext files") + } + } + } + + test("native text read - strips a leading UTF-8 BOM in line mode") { + withTempDir { dir => + val file = new File(dir, "bom.txt") + val bom = Array[Byte](0xef.toByte, 0xbb.toByte, 0xbf.toByte) + Files.write(file.toPath, bom ++ "hello\nworld".getBytes(StandardCharsets.UTF_8)) + withTextConf { + // Spark's line reader strips the BOM; native must match. + checkSparkAnswerAndOperator(spark.read.text(file.getAbsolutePath)) + } + } + } + + test("native text read - fallback for ignoreCorruptFiles / ignoreMissingFiles") { + withTempDir { dir => + val file = new File(dir, "a.txt") + Files.write(file.toPath, "a\nb".getBytes(StandardCharsets.UTF_8)) + withTextConf { + checkSparkAnswerAndFallbackReason( + spark.read.option("ignoreCorruptFiles", "true").text(file.getAbsolutePath), + "Comet native Text scan does not support ignoreCorruptFiles") + checkSparkAnswerAndFallbackReason( + spark.read.option("ignoreMissingFiles", "true").text(file.getAbsolutePath), + "Comet native Text scan does not support ignoreMissingFiles") + } + } + } + + test("native text read - fallback for compressed (.gz) files") { + withTempDir { dir => + val file = new File(dir, "data.txt.gz") + val out = new java.util.zip.GZIPOutputStream(new java.io.FileOutputStream(file)) + out.write("a\nb\nc".getBytes(StandardCharsets.UTF_8)) + out.close() + withTextConf { + val df = spark.read.text(file.getAbsolutePath) + checkSparkAnswerAndFallbackReason(df, "Comet does not support compressed text files") + } + } + } +} From bdaddc0390d21c96ace83037c37ed3bc07c9e1b8 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Fri, 9 Oct 2026 15:54:32 -0700 Subject: [PATCH 02/12] address review comments --- .../user-guide/latest/compatibility/scans.md | 5 +- docs/source/user-guide/latest/datasources.md | 11 +- .../latest/understanding-comet-plans.md | 1 + .../scala/org/apache/comet/CometConf.scala | 10 ++ .../apache/comet/rules/CometScanRule.scala | 50 ++++++-- .../sql/comet/CometTextNativeScanExec.scala | 5 + .../apache/comet/shims/ShimFileFormat.scala | 8 ++ .../apache/comet/shims/ShimFileFormat.scala | 9 ++ .../apache/comet/shims/ShimFileFormat.scala | 9 ++ .../apache/comet/shims/ShimFileFormat.scala | 10 ++ .../apache/comet/shims/ShimFileFormat.scala | 10 ++ .../comet/text/CometTextNativeReadSuite.scala | 116 +++++++++++++++++- 12 files changed, 225 insertions(+), 19 deletions(-) diff --git a/docs/source/user-guide/latest/compatibility/scans.md b/docs/source/user-guide/latest/compatibility/scans.md index 7b7d2c198e8..17cd09dd396 100644 --- a/docs/source/user-guide/latest/compatibility/scans.md +++ b/docs/source/user-guide/latest/compatibility/scans.md @@ -89,8 +89,9 @@ The following limitations raise an error at scan time rather than falling back t query, or cast the column to `BINARY` before persisting, if you need to preserve non-UTF-8 bytes. By contrast, Comet decodes non-UTF-8 bytes at the JVM-to-native Arrow FFI boundaries using JVM-compatible replacement semantics. This covers native query input from the JVM-exported Arrow - stream (including JVM scans, shuffle reads, and `mapInArrow`), columnar-to-row conversion, and JVM - UDF results. It does not change the native Parquet reader: rejecting invalid UTF-8 there remains + stream (including JVM scans, shuffle reads, and `mapInArrow`), columnar-to-row conversion, JVM + UDF results, and the native text scan (which replaces invalid bytes in each line's `value` with + U+FFFD). It does not change the native Parquet reader: rejecting invalid UTF-8 there remains Gap A of [#4764](https://github.com/apache/datafusion-comet/issues/4764). See [Strings with non-UTF-8 bytes](index.md#strings-with-non-utf-8-bytes), [#4121](https://github.com/apache/datafusion-comet/issues/4121), and the tracking issue above. diff --git a/docs/source/user-guide/latest/datasources.md b/docs/source/user-guide/latest/datasources.md index 6bc58d331b4..32d8498cab0 100644 --- a/docs/source/user-guide/latest/datasources.md +++ b/docs/source/user-guide/latest/datasources.md @@ -57,10 +57,15 @@ Comet provides experimental Rust-based text scan support (a single `value: strin `spark.comet.scan.text.v2.enabled` is enabled, text files are read in Rust. This feature is experimental and performance benefits are workload-dependent. Only Spark's DataSource V2 text scan is accelerated, and Spark reads text through the V1 API by default, so also remove `text` from `spark.sql.sources.useV1SourceList`. The native scan -falls back to Spark for cases it does not handle -- including partitioned tables, compressed files, files large -enough that Spark splits them into byte ranges, `wholetext` files larger than roughly 2GB (capped at -`min(spark.sql.files.maxPartitionBytes, 2GB)`), unsupported filesystem schemes, and `input_file_name()` or +falls back to Spark for cases it does not handle -- including compressed files, files larger than +`spark.comet.scan.text.maxFileSize` (64MB by default, since the reader loads each file whole), files Spark splits +into byte ranges, unsupported filesystem schemes, and `input_file_name()` or `ignoreCorruptFiles`/`ignoreMissingFiles` usage. It targets small lookup tables such as a broadcast join's build side. +Reading a partition column from a partitioned directory falls back to Spark, but selecting only `value` stays native. + +Text files are a common source of ill-formed UTF-8. The native reader replaces invalid byte sequences with the +Unicode replacement character (U+FFFD), so byte-level comparisons and functions can differ from Spark on those rows; +see [Strings with non-UTF-8 bytes](compatibility/scans.md) in the compatibility guide. ### Other Spark inputs diff --git a/docs/source/user-guide/latest/understanding-comet-plans.md b/docs/source/user-guide/latest/understanding-comet-plans.md index 4791f1abf5a..7c53a73c56c 100644 --- a/docs/source/user-guide/latest/understanding-comet-plans.md +++ b/docs/source/user-guide/latest/understanding-comet-plans.md @@ -298,6 +298,7 @@ by role. Names match what is shown in the plan output. | `CometNativeScan` | Parquet scan that runs entirely in Rust via DataFusion. | | `CometIcebergNativeScan` | Iceberg Parquet scan that runs entirely in Rust via DataFusion. | | `CometCsvNativeScan` | CSV scan that runs entirely in Rust via DataFusion (experimental). | +| `CometTextNativeScan` | Text scan (single `value` string column) that runs entirely in Rust via DataFusion (experimental). | | `CometInMemoryTableScan` | JVM-side scan of a table cached in Comet's Arrow format (enabled by default from Spark 3.5). | | `CometLocalTableScan` | JVM-side scan of a `LocalTableScanExec` (disabled by default). | | `CometEmptyRelation` | Empty native input that replaces `EmptyRelationExec` (Spark 4.0 and later). | diff --git a/spark/src/main/scala/org/apache/comet/CometConf.scala b/spark/src/main/scala/org/apache/comet/CometConf.scala index cfec05d3301..1a90591feb0 100644 --- a/spark/src/main/scala/org/apache/comet/CometConf.scala +++ b/spark/src/main/scala/org/apache/comet/CometConf.scala @@ -182,6 +182,16 @@ object CometConf extends ShimCometConf { .booleanConf .createWithDefault(false) + val COMET_SCAN_TEXT_MAX_FILE_SIZE: ConfigEntry[Long] = + conf("spark.comet.scan.text.maxFileSize") + .category(CATEGORY_TESTING) + .doc("Maximum size of a single text file that the native Text scan will read. The native " + + "reader loads each file whole and does not reserve the buffer against the memory pool, " + + "so this bounds its peak memory; files larger than this fall back to Spark. The native " + + "Text scan targets small lookup tables such as a broadcast join's build side.") + .bytesConf(ByteUnit.BYTE) + .createWithDefault(64L * 1024 * 1024) + val COMET_PARQUET_ROW_FILTER_PUSHDOWN_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.parquet.rowFilterPushdown.enabled") .category(CATEGORY_PARQUET) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index 65d55d6b2c4..9435a41a4c3 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -483,13 +483,9 @@ case class CometScanRule(session: SparkSession) s"Native Text scan requires ${COMET_EXEC_ENABLED.key} to be enabled") } // input_file_name/_block_start/_block_length read a thread-local that Spark's FileScanRDD - // sets; the native DataFusion scan does not, so they would return "" / -1. Same guard as - // the native Parquet path. - if (plan.exists(node => - node.expressions.exists(_.exists { - case _: InputFileName | _: InputFileBlockStart | _: InputFileBlockLength => true - case _ => false - }))) { + // sets; the native DataFusion scan does not, so they would return "" / -1. Reuse the shared + // helper the Parquet gate uses so the two cannot drift. + if (CometScanRule.readsInputFileBlock(plan)) { return withFallbackReason( scanExec, "Native Text scan is not compatible with input_file_name, " + @@ -524,14 +520,26 @@ case class CometScanRule(session: SparkSession) fallbackReasons += "Comet native Text scan does not support files split into byte " + "ranges (large files)" } + // The native reader loads each file whole and does not reserve the buffer against the + // memory pool, so bound the per-file size; larger files fall back to Spark. This keeps the + // feature to the small lookup tables it targets regardless of maxPartitionBytes. + val maxTextFileSize = COMET_SCAN_TEXT_MAX_FILE_SIZE.get() + val hasOversizedFile = scanExec.inputPartitions.exists { + case fp: FilePartition => fp.files.exists(_.length > maxTextFileSize) + case _ => false + } + if (hasOversizedFile) { + fallbackReasons += "Comet native Text scan does not read files larger than " + + s"${COMET_SCAN_TEXT_MAX_FILE_SIZE.key} ($maxTextFileSize bytes)" + } // Comet's native plan does not carry a compression codec, so the native reader would feed - // raw compressed bytes to the line splitter. Spark auto-detects a codec by file extension - // (e.g. .gz, .bz2). Fall back when any input file is compressed. Use the options-aware - // Hadoop conf so a codec registered via a per-read option is honored (as Spark does). + // raw compressed bytes to the line splitter. Spark auto-detects a codec by file extension. + // Use the options-aware Hadoop conf (so a per-read codec option is honored) and the + // version-aware ShimFileFormat.isCompressedFile, which on Spark 4.1+ also catches the + // non-standard .gzip/.zstd extensions that CompressionCodecFactory alone misses. val hadoopConf = session.sessionState.newHadoopConfWithOptions(scan.options.asScala.toMap) - val codecFactory = new org.apache.hadoop.io.compress.CompressionCodecFactory(hadoopConf) val hasCompressedFile = scan.fileIndex.inputFiles.exists { path => - codecFactory.getCodec(new org.apache.hadoop.fs.Path(path)) != null + ShimFileFormat.isCompressedFile(hadoopConf, new org.apache.hadoop.fs.Path(path)) } if (hasCompressedFile) { fallbackReasons += "Comet does not support compressed text files" @@ -548,6 +556,15 @@ case class CometScanRule(session: SparkSession) false)) { fallbackReasons += "Comet native Text scan does not support ignoreMissingFiles" } + // Spark's line reader skips any line at or over + // mapreduce.input.linerecordreader.line.maxlength (default unlimited); the key can come + // from spark.hadoop.*, core-site.xml, or a read option. The native reader does not + // truncate, so fall back when a finite limit is set. + if (hadoopConf.getInt("mapreduce.input.linerecordreader.line.maxlength", Int.MaxValue) + != Int.MaxValue) { + fallbackReasons += "Comet native Text scan does not support " + + "mapreduce.input.linerecordreader.line.maxlength" + } // A wholetext file is never split (TextScan.isSplitable is false for wholeText), so the // split-file guard above does not bound it. Reading a very large file whole is unbounded // memory, and the whole file becomes one Arrow string value. Because a lossy UTF-8 decode @@ -602,6 +619,15 @@ case class CometScanRule(session: SparkSession) fallbackReasons += s"Native Text scan cannot open path '${rejectedPath.get.uri}': " + "object_store rejects it (e.g. an unsupported character in the path)" } + // Native planning registers one object store per partition and strips the bucket from every + // key, so an S3-compliant alias scan spanning multiple buckets would read every file from + // the first bucket. Decline multi-bucket alias scans (same hazard the Parquet gate guards). + val scanBuckets = CometScanRule.aliasScanBuckets(roots) + if (scanBuckets.size > 1) { + fallbackReasons += "Native Text scan reads S3-compliant alias paths across multiple " + + s"buckets (${scanBuckets.toSeq.sorted.mkString(", ")}); Comet registers one object " + + "store per file partition and would read every file from the first file's bucket" + } if (fallbackReasons.isEmpty) { CometBatchScanExec( scanExec.clone().asInstanceOf[BatchScanExec], diff --git a/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala index 3b46167e218..ced8fa75310 100644 --- a/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala +++ b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala @@ -51,6 +51,11 @@ case class CometTextNativeScanExec( override val nodeName: String = "CometTextNativeScan" + // Show only the semantic output in EXPLAIN / the SQL tab / the event log. The default would print + // the whole `nativeOp` protobuf, which carries every input file path (large for a lookup + // directory of many files). Mirrors CometNativeScanExec. + override def stringArgs: Iterator[Any] = Iterator(output) + override def outputPartitioning: Partitioning = UnknownPartitioning( originalPlan.inputPartitions.length) diff --git a/spark/src/main/spark-3.4/org/apache/comet/shims/ShimFileFormat.scala b/spark/src/main/spark-3.4/org/apache/comet/shims/ShimFileFormat.scala index 6906a39fbfc..c80b57f7664 100644 --- a/spark/src/main/spark-3.4/org/apache/comet/shims/ShimFileFormat.scala +++ b/spark/src/main/spark-3.4/org/apache/comet/shims/ShimFileFormat.scala @@ -19,7 +19,9 @@ package org.apache.comet.shims +import org.apache.hadoop.conf.Configuration import org.apache.hadoop.fs.Path +import org.apache.hadoop.io.compress.CompressionCodecFactory import org.apache.spark.sql.catalyst.expressions.Literal import org.apache.spark.sql.execution.datasources.{FileFormat, PartitionedFile, RowIndexUtil} import org.apache.spark.sql.types.{DataType, StructType} @@ -30,6 +32,12 @@ object ShimFileFormat { // until they can be placed in the _metadata struct. val ROW_INDEX_TEMPORARY_COLUMN_NAME: String = FileFormat.ROW_INDEX_TEMPORARY_COLUMN_NAME + // Whether Spark's text/line readers would decompress this file. Before Spark 4.1 the codec is + // resolved purely through CompressionCodecFactory (the non-standard `.gzip`/`.zstd` extensions + // are read raw on these versions, so they are intentionally not treated as compressed here). + def isCompressedFile(hadoopConf: Configuration, path: Path): Boolean = + new CompressionCodecFactory(hadoopConf).getCodec(path) != null + def findRowIndexColumnIndexInSchema(sparkSchema: StructType): Int = RowIndexUtil.findRowIndexColumnIndexInSchema(sparkSchema) diff --git a/spark/src/main/spark-3.5/org/apache/comet/shims/ShimFileFormat.scala b/spark/src/main/spark-3.5/org/apache/comet/shims/ShimFileFormat.scala index 9506a851c08..9210a29d5ce 100644 --- a/spark/src/main/spark-3.5/org/apache/comet/shims/ShimFileFormat.scala +++ b/spark/src/main/spark-3.5/org/apache/comet/shims/ShimFileFormat.scala @@ -19,6 +19,9 @@ package org.apache.comet.shims +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.Path +import org.apache.hadoop.io.compress.CompressionCodecFactory import org.apache.spark.sql.catalyst.expressions.Literal import org.apache.spark.sql.execution.datasources.{FileFormat, PartitionedFile} import org.apache.spark.sql.execution.datasources.parquet.ParquetFileFormat @@ -30,6 +33,12 @@ object ShimFileFormat { // until they can be placed in the _metadata struct. val ROW_INDEX_TEMPORARY_COLUMN_NAME = ParquetFileFormat.ROW_INDEX_TEMPORARY_COLUMN_NAME + // Whether Spark's text/line readers would decompress this file. Before Spark 4.1 the codec is + // resolved purely through CompressionCodecFactory (the non-standard `.gzip`/`.zstd` extensions + // are read raw on these versions, so they are intentionally not treated as compressed here). + def isCompressedFile(hadoopConf: Configuration, path: Path): Boolean = + new CompressionCodecFactory(hadoopConf).getCodec(path) != null + def findRowIndexColumnIndexInSchema(sparkSchema: StructType): Int = ParquetRowIndexUtil.findRowIndexColumnIndexInSchema(sparkSchema) diff --git a/spark/src/main/spark-4.0/org/apache/comet/shims/ShimFileFormat.scala b/spark/src/main/spark-4.0/org/apache/comet/shims/ShimFileFormat.scala index 9506a851c08..9210a29d5ce 100644 --- a/spark/src/main/spark-4.0/org/apache/comet/shims/ShimFileFormat.scala +++ b/spark/src/main/spark-4.0/org/apache/comet/shims/ShimFileFormat.scala @@ -19,6 +19,9 @@ package org.apache.comet.shims +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.Path +import org.apache.hadoop.io.compress.CompressionCodecFactory import org.apache.spark.sql.catalyst.expressions.Literal import org.apache.spark.sql.execution.datasources.{FileFormat, PartitionedFile} import org.apache.spark.sql.execution.datasources.parquet.ParquetFileFormat @@ -30,6 +33,12 @@ object ShimFileFormat { // until they can be placed in the _metadata struct. val ROW_INDEX_TEMPORARY_COLUMN_NAME = ParquetFileFormat.ROW_INDEX_TEMPORARY_COLUMN_NAME + // Whether Spark's text/line readers would decompress this file. Before Spark 4.1 the codec is + // resolved purely through CompressionCodecFactory (the non-standard `.gzip`/`.zstd` extensions + // are read raw on these versions, so they are intentionally not treated as compressed here). + def isCompressedFile(hadoopConf: Configuration, path: Path): Boolean = + new CompressionCodecFactory(hadoopConf).getCodec(path) != null + def findRowIndexColumnIndexInSchema(sparkSchema: StructType): Int = ParquetRowIndexUtil.findRowIndexColumnIndexInSchema(sparkSchema) diff --git a/spark/src/main/spark-4.1/org/apache/comet/shims/ShimFileFormat.scala b/spark/src/main/spark-4.1/org/apache/comet/shims/ShimFileFormat.scala index 9506a851c08..70bc5337baf 100644 --- a/spark/src/main/spark-4.1/org/apache/comet/shims/ShimFileFormat.scala +++ b/spark/src/main/spark-4.1/org/apache/comet/shims/ShimFileFormat.scala @@ -19,6 +19,9 @@ package org.apache.comet.shims +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.Path +import org.apache.spark.io.HadoopCodecStreams import org.apache.spark.sql.catalyst.expressions.Literal import org.apache.spark.sql.execution.datasources.{FileFormat, PartitionedFile} import org.apache.spark.sql.execution.datasources.parquet.ParquetFileFormat @@ -30,6 +33,13 @@ object ShimFileFormat { // until they can be placed in the _metadata struct. val ROW_INDEX_TEMPORARY_COLUMN_NAME = ParquetFileFormat.ROW_INDEX_TEMPORARY_COLUMN_NAME + // Whether Spark's text/line readers would decompress this file. Spark 4.1+ resolves the codec + // through HadoopCodecStreams, which also treats the non-standard `.gzip` and `.zstd` extensions + // as compressed (CompressionCodecFactory alone misses those). Keep this in lockstep with the + // Spark version so the native text scan declines exactly the files Spark would decompress. + def isCompressedFile(hadoopConf: Configuration, path: Path): Boolean = + HadoopCodecStreams.getDecompressionCodec(hadoopConf, path).isDefined + def findRowIndexColumnIndexInSchema(sparkSchema: StructType): Int = ParquetRowIndexUtil.findRowIndexColumnIndexInSchema(sparkSchema) diff --git a/spark/src/main/spark-4.2/org/apache/comet/shims/ShimFileFormat.scala b/spark/src/main/spark-4.2/org/apache/comet/shims/ShimFileFormat.scala index 4aee5671de8..4e4c3152ad2 100644 --- a/spark/src/main/spark-4.2/org/apache/comet/shims/ShimFileFormat.scala +++ b/spark/src/main/spark-4.2/org/apache/comet/shims/ShimFileFormat.scala @@ -19,6 +19,9 @@ package org.apache.comet.shims +import org.apache.hadoop.conf.Configuration +import org.apache.hadoop.fs.Path +import org.apache.spark.io.HadoopCodecStreams import org.apache.spark.sql.catalyst.expressions.Literal import org.apache.spark.sql.execution.datasources.{FileFormat, PartitionedFile} import org.apache.spark.sql.execution.datasources.parquet.ParquetFileFormat @@ -30,6 +33,13 @@ object ShimFileFormat { // until they can be placed in the _metadata struct. val ROW_INDEX_TEMPORARY_COLUMN_NAME = ParquetFileFormat.ROW_INDEX_TEMPORARY_COLUMN_NAME + // Whether Spark's text/line readers would decompress this file. Spark 4.1+ resolves the codec + // through HadoopCodecStreams, which also treats the non-standard `.gzip` and `.zstd` extensions + // as compressed (CompressionCodecFactory alone misses those). Keep this in lockstep with the + // Spark version so the native text scan declines exactly the files Spark would decompress. + def isCompressedFile(hadoopConf: Configuration, path: Path): Boolean = + HadoopCodecStreams.getDecompressionCodec(hadoopConf, path).isDefined + def findRowIndexColumnIndexInSchema(sparkSchema: StructType): Int = ParquetRowIndexUtil.findRowIndexColumnIndexInSchema(sparkSchema) diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index e9d73aa1b0f..c1bc243c5e6 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -24,10 +24,12 @@ import java.nio.charset.StandardCharsets import java.nio.file.Files import org.apache.spark.sql.CometTestBase +import org.apache.spark.sql.comet.CometTextNativeScanExec import org.apache.spark.sql.functions.input_file_name import org.apache.spark.sql.internal.SQLConf import org.apache.comet.CometConf +import org.apache.comet.CometSparkSessionExtensions.isSpark41Plus class CometTextNativeReadSuite extends CometTestBase { private val TEST_TEXT_PATH = "src/test/resources/test-data/text-test-1.txt" @@ -35,7 +37,9 @@ class CometTextNativeReadSuite extends CometTestBase { private def withTextConf(f: => Unit): Unit = { withSQLConf( CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", - SQLConf.USE_V1_SOURCE_LIST.key -> "")(f) + // Route only `text` through the V2 path; keep every other format on V1 so a Parquet probe + // side (used by the broadcast-join test) is not pushed through the unsupported V2 scan. + SQLConf.USE_V1_SOURCE_LIST.key -> "avro,csv,json,kafka,orc,parquet")(f) } test("native text read - lines to value column") { @@ -65,8 +69,11 @@ class CometTextNativeReadSuite extends CometTestBase { val lines = (1 to 500).map(i => s"row-$i") Files.write(file.toPath, lines.mkString("\n").getBytes(StandardCharsets.UTF_8)) withTextConf { - val got = spark.read.text(file.getAbsolutePath).collect().map(_.getString(0)).toSeq + val df = spark.read.text(file.getAbsolutePath) + // Assert the scan actually ran natively (a Spark fallback would also preserve order). + checkSparkAnswerAndOperator(df, Seq(classOf[CometTextNativeScanExec])) // checkSparkAnswer sorts both sides, so assert order explicitly here. + val got = df.collect().map(_.getString(0)).toSeq assert(got == lines, "native text scan must preserve within-file line order") } } @@ -276,4 +283,109 @@ class CometTextNativeReadSuite extends CometTestBase { } } } + + test("native text read - fallback for .gzip/.zstd compressed files on Spark 4.1+") { + // Spark 4.1+ decompresses the non-standard .gzip/.zstd extensions (via HadoopCodecStreams); + // CompressionCodecFactory alone misses them, so the version-aware shim must catch them. + assume(isSpark41Plus, "Spark 4.1+ treats .gzip/.zstd as compressed") + withTempDir { dir => + val gzip = new File(dir, "data.txt.gzip") + val out = new java.util.zip.GZIPOutputStream(new java.io.FileOutputStream(gzip)) + out.write("a\nb\nc".getBytes(StandardCharsets.UTF_8)) + out.close() + withTextConf { + checkSparkAnswerAndFallbackReason( + spark.read.text(gzip.getAbsolutePath), + "Comet does not support compressed text files") + } + } + } + + test("native text read - fallback for line.maxlength") { + withTempDir { dir => + val file = new File(dir, "lines.txt") + Files.write(file.toPath, "ab\nabcdefghij\nxy".getBytes(StandardCharsets.UTF_8)) + withTextConf { + // As a per-read option the key reaches the scan's Hadoop conf (stripped), so Spark's line + // reader skips lines >= this length; the native reader does not, so Comet must fall back. + checkSparkAnswerAndFallbackReason( + spark.read + .option("mapreduce.input.linerecordreader.line.maxlength", "5") + .text(file.getAbsolutePath), + "mapreduce.input.linerecordreader.line.maxlength") + } + } + } + + test("native text read - fallback for files larger than the size ceiling") { + withTempDir { dir => + val file = new File(dir, "big.txt") + Files.write(file.toPath, ("x\n" * 1000).getBytes(StandardCharsets.UTF_8)) + withSQLConf( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "avro,csv,json,kafka,orc,parquet", + // Tiny ceiling so the ~2KB file exceeds it. + CometConf.COMET_SCAN_TEXT_MAX_FILE_SIZE.key -> "100") { + checkSparkAnswerAndFallbackReason( + spark.read.text(file.getAbsolutePath), + "does not read files larger than") + } + } + } + + test("native text read - partition column fallback: value stays native, select * falls back") { + withTempDir { dir => + // Partitioned layout: /p=1/a.txt + val part = new File(dir, "p=1") + part.mkdirs() + Files.write(new File(part, "a.txt").toPath, "x\ny".getBytes(StandardCharsets.UTF_8)) + withTextConf { + // Selecting only `value` has no partition column -> stays native. + checkSparkAnswerAndOperator(spark.read.text(dir.getAbsolutePath).select("value")) + // Selecting the partition column `p` -> falls back to Spark. + checkSparkAnswerAndFallbackReason( + spark.read.text(dir.getAbsolutePath).selectExpr("value", "p"), + "Comet does not support partition columns in native Text scans") + } + } + } + + test("native text read - multiple unsplit files across separate partitions") { + withTempDir { dir => + (1 to 6).foreach { i => + Files.write( + new File(dir, s"f$i.txt").toPath, + s"a$i\nb$i".getBytes(StandardCharsets.UTF_8)) + } + withSQLConf( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "avro,csv,json,kafka,orc,parquet", + // A large open cost keeps each small file in its own partition, exercising + // file_partitions[self.partition] beyond index 0. + SQLConf.FILES_MAX_PARTITION_BYTES.key -> "8", + SQLConf.FILES_OPEN_COST_IN_BYTES.key -> "1073741824") { + val df = spark.read.text(dir.getAbsolutePath) + assert(df.rdd.getNumPartitions > 1, "test requires files in separate partitions") + checkSparkAnswerAndOperator(df) + } + } + } + + test("native text read - broadcast join with a text build side runs natively") { + withTempDir { dir => + val lookup = new File(dir, "allow.txt") + Files.write(lookup.toPath, "k1\nk2\nk3".getBytes(StandardCharsets.UTF_8)) + withSQLConf( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "avro,csv,json,kafka,orc,parquet") { + withParquetTable((1 to 100).map(i => (i, s"k${i % 5}")), "facts") { + val lookupDf = spark.read.text(lookup.getAbsolutePath) + val facts = spark.table("facts") + // Small text build side broadcast-joined to a native Parquet probe side. + val joined = facts.join(lookupDf.hint("broadcast"), facts("_2") === lookupDf("value")) + checkSparkAnswerAndOperator(joined) + } + } + } + } } From 49fc103c14a1347080c60cddd4f071c11b6fa4fe Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Fri, 9 Oct 2026 16:44:12 -0700 Subject: [PATCH 03/12] more comments addressed --- .../apache/comet/rules/CometScanRule.scala | 168 ++++++++---------- .../sql/comet/CometCsvNativeScanExec.scala | 5 + 2 files changed, 77 insertions(+), 96 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index 9435a41a4c3..d8e4abd33ae 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -246,77 +246,31 @@ case class CometScanRule(session: SparkSession) return None } // Comet's native readers go through object_store, which only understands a fixed set of URL - // schemes. A custom Hadoop FileSystem (e.g. registered via spark.hadoop.fs..impl) would - // surface at execution time as `Generic URL error: Unable to recognise URL "..."`. Decline here - // so Spark's reader -- which goes through the Hadoop FS API and can resolve custom schemes -- - // handles the scan. Whether object_store recognizes a scheme is answered by the native layer - // itself (`NativeBase.isObjectStoreSchemeSupported`) rather than a hardcoded list, so the - // planner can't drift from object_store's actual support. - // - // EXCEPT schemes the user routes through libhdfs via `spark.hadoop.fs.comet.libhdfs.schemes` - // (e.g. `hdfs`, or a test `fake`): those ARE natively readable through the libhdfs object_store - // bridge, so they must NOT be declined here. The claim decision is guarded in CI by - // CometScanSchemeFallbackSuite; end-to-end execution through libhdfs is guarded by - // ParquetReadFromFakeHadoopFsSuite, which is a manual suite (see its scaladoc). - // - // The default mirrors the native side: when the config is unset, `is_hdfs_scheme` - // (native/core/src/parquet/parquet_support.rs) treats `hdfs` as natively readable, and - // `create_hdfs_object_store` is in the default build (`default = ["hdfs-opendal"]`). If we - // defaulted to an empty set here, a plain `hdfs://` V1 scan would be declined and fall back to - // Spark even though native can read it -- a silent regression for HDFS users in the default - // configuration. So default to `Set("hdfs")` to stay in lockstep with the native default. - val libhdfsSchemes: Set[String] = COMET_LIBHDFS_SCHEMES.get() match { - case Some(s) => NativeConfig.parseSchemeSet(s) - case None => Set("hdfs") - } - // Opt-in S3-compliant alias schemes (e.g. `blob`) from `fs.comet.s3Compliant.schemes`. Read - // from the Hadoop config rather than SQLConf (unlike COMET_LIBHDFS_SCHEMES above) so - // `core-site.xml` is honored. The native object_store gate no longer claims aliases, so admit - // them here where the Hadoop config is available. - val s3CompliantSchemes = NativeConfig.resolveS3CompliantSchemes(hadoopConf) - - // Classify each root path once; see RootPathInfo. - val roots = CometScanRule.classifyRootPaths( - r.location.rootPaths.map(_.toUri), - libhdfsSchemes, - s3CompliantSchemes) - - val unsupportedFsSchemes = roots.iterator.flatMap { root => - root.scheme.filter(_ => - !root.isLibhdfs && !CometScanRule.isNativelyReadableScheme(root.uri, s3CompliantSchemes)) - }.toSet - if (unsupportedFsSchemes.nonEmpty) { + // schemes; a custom Hadoop FileSystem would hard-fail at execution. The shared helper + // classifies the root paths (honoring libhdfs and S3-compliant aliases) and reports + // unsupported schemes, multi-bucket alias scans, and object_store-rejected paths. The scheme + // claim decision is guarded in CI by CometScanSchemeFallbackSuite; libhdfs end-to-end by + // ParquetReadFromFakeHadoopFsSuite. + val rootIssues = + CometScanRule.checkObjectStoreRootPaths(r.location.rootPaths.map(_.toUri), hadoopConf) + if (rootIssues.unsupportedSchemes.nonEmpty) { withFallbackReason( scanExec, - s"Unsupported filesystem schemes: ${unsupportedFsSchemes.mkString(", ")}") + s"Unsupported filesystem schemes: ${rootIssues.unsupportedSchemes.mkString(", ")}") return None } - // More than one bucket cannot be served by the single object store native planning registers - // per FilePartition; see aliasScanBuckets. Scoped to alias scans: plain multi-bucket `s3://` - // has the same flaw today and silently declining it would newly fall back scans that work by - // luck, so that widening is left as a separate decision. Note the sibling Iceberg guard - // (dataFileBuckets, below) is NOT so scoped -- it declines multi-bucket `s3a://` too. - val scanBuckets = CometScanRule.aliasScanBuckets(roots) - if (scanBuckets.size > 1) { + if (rootIssues.multiBucket.nonEmpty) { withFallbackReason( scanExec, "Native Parquet scan reads S3-compliant alias paths across multiple buckets " + - s"(${scanBuckets.toSeq.sorted.mkString(", ")}); Comet registers one object store " + - "per file partition and would read every file from the first file's bucket") + s"(${rootIssues.multiBucket.toSeq.sorted.mkString(", ")}); Comet registers one object " + + "store per file partition and would read every file from the first file's bucket") return None } - // A scheme object_store recognizes can still carry a path it rejects (e.g. a directory whose - // name contains a newline -> `%0A` in the URI), which native execution would hard-fail on. - // Only object_store-native schemes are probed: aliases (normalized to s3:// natively) and - // libhdfs schemes route elsewhere. Root paths only -- a rejected character deeper in the tree - // still fails at execution. - val rejectedPath = roots.find(root => - root.scheme.isDefined && !root.isLibhdfs && !root.isAlias && - !CometScanRule.objectStoreAcceptsPath(root.uri)) - if (rejectedPath.nonEmpty) { + if (rootIssues.rejectedPath.nonEmpty) { withFallbackReason( scanExec, - s"Native Parquet scan cannot open path '${rejectedPath.get.uri}': object_store " + + s"Native Parquet scan cannot open path '${rootIssues.rejectedPath.get}': object_store " + "rejects it (e.g. an unsupported character in the path)") return None } @@ -587,46 +541,25 @@ case class CometScanRule(session: SparkSession) } } // Comet's native reader opens files through object_store, which only understands a fixed - // set of URL schemes. A text file on a custom Hadoop scheme (viewfs://, oss://, ...) would - // be claimed here then hard-fail at execution, whereas Spark reads it via the Hadoop FS - // API. Decline such schemes (same gate as the native Parquet path); hdfs:// routes through - // libhdfs and is left to native. - val libhdfsSchemes: Set[String] = COMET_LIBHDFS_SCHEMES.get() match { - case Some(s) => NativeConfig.parseSchemeSet(s) - case None => Set("hdfs") - } - val s3CompliantSchemes = NativeConfig.resolveS3CompliantSchemes(hadoopConf) - val roots = CometScanRule.classifyRootPaths( + // set of URL schemes. A text file on a custom Hadoop scheme (viewfs://, oss://, ...), a + // multi-bucket S3-compliant alias scan, or a path object_store rejects would be claimed + // here then hard-fail at execution, whereas Spark reads it via the Hadoop FS API. Use the + // same shared gate as the native Parquet path; hdfs:// routes through libhdfs. + val rootIssues = CometScanRule.checkObjectStoreRootPaths( scan.fileIndex.rootPaths.map(_.toUri), - libhdfsSchemes, - s3CompliantSchemes) - val unsupportedFsSchemes = roots.iterator.flatMap { root => - root.scheme.filter(_ => - !root.isLibhdfs && !CometScanRule.isNativelyReadableScheme( - root.uri, - s3CompliantSchemes)) - }.toSet - if (unsupportedFsSchemes.nonEmpty) { + hadoopConf) + if (rootIssues.unsupportedSchemes.nonEmpty) { fallbackReasons += - s"Unsupported filesystem schemes: ${unsupportedFsSchemes.mkString(", ")}" - } - // A recognized scheme can still carry a path object_store rejects (e.g. a newline -> %0A), - // which native execution would hard-fail on. Root paths only. - val rejectedPath = roots.find(root => - root.scheme.isDefined && !root.isLibhdfs && !root.isAlias && - !CometScanRule.objectStoreAcceptsPath(root.uri)) - if (rejectedPath.nonEmpty) { - fallbackReasons += s"Native Text scan cannot open path '${rejectedPath.get.uri}': " + - "object_store rejects it (e.g. an unsupported character in the path)" + s"Unsupported filesystem schemes: ${rootIssues.unsupportedSchemes.mkString(", ")}" } - // Native planning registers one object store per partition and strips the bucket from every - // key, so an S3-compliant alias scan spanning multiple buckets would read every file from - // the first bucket. Decline multi-bucket alias scans (same hazard the Parquet gate guards). - val scanBuckets = CometScanRule.aliasScanBuckets(roots) - if (scanBuckets.size > 1) { + if (rootIssues.multiBucket.nonEmpty) { fallbackReasons += "Native Text scan reads S3-compliant alias paths across multiple " + - s"buckets (${scanBuckets.toSeq.sorted.mkString(", ")}); Comet registers one object " + - "store per file partition and would read every file from the first file's bucket" + s"buckets (${rootIssues.multiBucket.toSeq.sorted.mkString(", ")}); Comet registers " + + "one object store per file partition and would read every file from the first bucket" + } + rootIssues.rejectedPath.foreach { uri => + fallbackReasons += s"Native Text scan cannot open path '$uri': " + + "object_store rejects it (e.g. an unsupported character in the path)" } if (fallbackReasons.isEmpty) { CometBatchScanExec( @@ -1433,6 +1366,49 @@ object CometScanRule extends Logging { bucket = NativeConfig.bucketForUri(uri, s3CompliantSchemes)) } + /** + * The object_store scheme/bucket/path problems that make a set of root paths unreadable by + * Comet's native file scans (shared by the Parquet V1 gate and the Text V2 gate). Each caller + * formats its own fallback message from these findings. + * + * - `unsupportedSchemes`: schemes object_store does not natively read and that are not routed + * through libhdfs (a custom Hadoop FileSystem would hard-fail at execution). + * - `multiBucket`: the S3-compliant alias buckets when a scan spans more than one; native + * planning registers one object store per FilePartition and would read every file from the + * first file's bucket. Empty when a single bucket (or no alias) is used. + * - `rejectedPath`: a root whose recognized scheme carries a path object_store rejects (e.g. + * a newline -> `%0A`), which native execution cannot open. + */ + private[rules] case class RootPathIssues( + unsupportedSchemes: Set[String], + multiBucket: Set[String], + rejectedPath: Option[URI]) + + private[rules] def checkObjectStoreRootPaths( + rootUris: Seq[URI], + hadoopConf: Configuration): RootPathIssues = { + // Schemes routed through libhdfs are natively readable; default to Set("hdfs") to match the + // native side (see the Parquet gate's comment). S3-compliant aliases come from the Hadoop + // config so core-site.xml is honored. + val libhdfsSchemes: Set[String] = COMET_LIBHDFS_SCHEMES.get() match { + case Some(s) => NativeConfig.parseSchemeSet(s) + case None => Set("hdfs") + } + val s3CompliantSchemes = NativeConfig.resolveS3CompliantSchemes(hadoopConf) + val roots = classifyRootPaths(rootUris, libhdfsSchemes, s3CompliantSchemes) + val unsupportedSchemes = roots.iterator.flatMap { root => + root.scheme.filter(_ => + !root.isLibhdfs && !isNativelyReadableScheme(root.uri, s3CompliantSchemes)) + }.toSet + val buckets = aliasScanBuckets(roots) + val rejectedPath = roots + .find(root => + root.scheme.isDefined && !root.isLibhdfs && !root.isAlias && + !objectStoreAcceptsPath(root.uri)) + .map(_.uri) + RootPathIssues(unsupportedSchemes, if (buckets.size > 1) buckets else Set.empty, rejectedPath) + } + /** * The distinct buckets this scan's root paths address, or empty when none of them uses an * opt-in S3-compliant alias. More than one bucket means the Parquet gate must fall back: native diff --git a/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala b/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala index 3d9f3f773fc..42f13120b80 100644 --- a/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala +++ b/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala @@ -51,6 +51,11 @@ case class CometCsvNativeScanExec( override val nodeName: String = "CometCsvNativeScan" + // Show only the semantic output in EXPLAIN / the SQL tab / the event log; the default would + // print the whole `nativeOp` protobuf, which carries every input file path. Mirrors + // CometNativeScanExec. + override def stringArgs: Iterator[Any] = Iterator(output) + override def outputPartitioning: Partitioning = UnknownPartitioning( originalPlan.inputPartitions.length) From a705210d1677e454a781d961ef853e4f04fcadb9 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Mon, 12 Oct 2026 12:55:49 -0700 Subject: [PATCH 04/12] address second review round: BOM-vs-lineSep, per-read hadoop options, multi-bucket text gate - text_scan.rs: strip a leading UTF-8 BOM only with the default separator (a custom lineSep leaves the BOM as data), matching Spark; update the unit test - CometTextNativeScanExec / CometCsvNativeScanExec: build the object-store Hadoop conf from the scan's read options (case preserved) so per-read settings like fs.s3a.endpoint reach native - CometScanRule: decline native Text scans whose roots span more than one scheme/bucket (RootPathIssues.crossLocations); Parquet gate behavior unchanged - tests: add .zstd fallback, BOM-vs-lineSep and BOM-only parity tests - docs: fix the non-UTF-8 compatibility link target --- docs/source/user-guide/latest/datasources.md | 2 +- .../core/src/execution/operators/text_scan.rs | 59 +++++++++++++------ .../apache/comet/rules/CometScanRule.scala | 33 +++++++++-- .../sql/comet/CometCsvNativeScanExec.scala | 4 +- .../sql/comet/CometTextNativeScanExec.scala | 5 +- .../comet/text/CometTextNativeReadSuite.scala | 39 +++++++++++- 6 files changed, 112 insertions(+), 30 deletions(-) diff --git a/docs/source/user-guide/latest/datasources.md b/docs/source/user-guide/latest/datasources.md index 32d8498cab0..053fc3ebfda 100644 --- a/docs/source/user-guide/latest/datasources.md +++ b/docs/source/user-guide/latest/datasources.md @@ -65,7 +65,7 @@ Reading a partition column from a partitioned directory falls back to Spark, but Text files are a common source of ill-formed UTF-8. The native reader replaces invalid byte sequences with the Unicode replacement character (U+FFFD), so byte-level comparisons and functions can differ from Spark on those rows; -see [Strings with non-UTF-8 bytes](compatibility/scans.md) in the compatibility guide. +see [Strings with non-UTF-8 bytes](compatibility/index.md#strings-with-non-utf-8-bytes) in the compatibility guide. ### Other Spark inputs diff --git a/native/core/src/execution/operators/text_scan.rs b/native/core/src/execution/operators/text_scan.rs index 743d63e0dc6..60a00d1b3b9 100644 --- a/native/core/src/execution/operators/text_scan.rs +++ b/native/core/src/execution/operators/text_scan.rs @@ -250,13 +250,13 @@ impl FileOpener for TextOpener { }; let mut timer = baseline_metrics.elapsed_compute().timer(); - // Spark's line reader (Hadoop LineRecordReader.skipUtfByteOrderMark) strips a leading - // UTF-8 BOM at the start of the file. Mirror that in line mode for the split that starts - // at file offset 0 (native only reads unsplit files, so start is always 0; a custom - // lineSep still strips, matching Hadoop). wholetext keeps the BOM, as Spark's - // WholeTextFileRecordReader does. + // Spark's line reader strips a leading UTF-8 BOM at the start of the file, but only with + // the default separator. With a custom lineSep the BOM is ordinary data (Spark leaves it + // for the splitter, so e.g. a file `BOM a BOM b` read with lineSep=BOM yields + // ["", "a", "b"]). Only strip in line mode, at file offset 0 (native reads unsplit files + // only), and when no custom lineSep is set. wholetext keeps the BOM. let at_file_start = range.as_ref().map(|r| r.start == 0).unwrap_or(true); - let content = strip_leading_bom(&buf, whole_text, at_file_start); + let content = strip_leading_bom(&buf, whole_text, at_file_start, line_sep.is_some()); let mut lines = split_lines(content, whole_text, line_sep.as_deref()); // Spark may split an uncompressed file into byte ranges (TextScan.isSplitable). A line // belongs to the split whose [start, end) contains the line's start offset -- so each @@ -275,10 +275,16 @@ impl FileOpener for TextOpener { } } -/// Strip a single leading UTF-8 BOM, matching Hadoop LineRecordReader.skipUtfByteOrderMark: only in -/// line mode (not wholetext) and only for the split that starts at file offset 0. -fn strip_leading_bom(buf: &[u8], whole_text: bool, at_file_start: bool) -> &[u8] { - if !whole_text && at_file_start && buf.starts_with(UTF8_BOM) { +/// Strip a single leading UTF-8 BOM the way Spark's default line reader does: only in line mode +/// (not wholetext), only for the split that starts at file offset 0, and only with the default +/// separator. With a custom lineSep the BOM is left in place as ordinary data. +fn strip_leading_bom( + buf: &[u8], + whole_text: bool, + at_file_start: bool, + custom_line_sep: bool, +) -> &[u8] { + if !whole_text && at_file_start && !custom_line_sep && buf.starts_with(UTF8_BOM) { &buf[UTF8_BOM.len()..] } else { buf @@ -544,23 +550,38 @@ mod tests { fn strips_leading_utf8_bom_in_line_mode() { let bom = [0xEFu8, 0xBB, 0xBF]; let with_hello = [&bom[..], b"hello"].concat(); - // Line mode at file start: the BOM is stripped. - assert_eq!(strip_leading_bom(&with_hello, false, true), b"hello"); + // Default separator at file start: the BOM is stripped. + assert_eq!(strip_leading_bom(&with_hello, false, true, false), b"hello"); // BOM then newline -> ["", "hello"] after split. let bom_nl = [&bom[..], b"\nhello"].concat(); assert_eq!( - lines_as_str(strip_leading_bom(&bom_nl, false, true), false, None), + lines_as_str(strip_leading_bom(&bom_nl, false, true, false), false, None), vec!["", "hello"] ); - // BOM-only file -> empty after strip -> zero lines. - assert!(split_lines(strip_leading_bom(&bom, false, true), false, None).is_empty()); - // Not stripped: wholetext mode, a non-start split, or no BOM present. - assert_eq!(strip_leading_bom(&with_hello, true, true), &with_hello[..]); + // BOM-only file, default separator -> empty after strip -> zero lines. + assert!(split_lines(strip_leading_bom(&bom, false, true, false), false, None).is_empty()); + // NOT stripped: a custom lineSep (BOM is data), so `BOM a BOM b` split on BOM is + // ["", "a", "b"]; wholetext; a non-start split; or no BOM present. + let bom_a_bom_b = [&bom[..], b"a", &bom[..], b"b"].concat(); + assert_eq!( + lines_as_str( + strip_leading_bom(&bom_a_bom_b, false, true, true), + false, + Some(&bom) + ), + vec!["", "a", "b"] + ); + // BOM-only file with lineSep=BOM -> one empty record (trailing separator dropped). + assert_eq!( + lines_as_str(strip_leading_bom(&bom, false, true, true), false, Some(&bom)), + vec![""] + ); + assert_eq!(strip_leading_bom(&with_hello, true, true, false), &with_hello[..]); assert_eq!( - strip_leading_bom(&with_hello, false, false), + strip_leading_bom(&with_hello, false, false, false), &with_hello[..] ); - assert_eq!(strip_leading_bom(b"hello", false, true), b"hello"); + assert_eq!(strip_leading_bom(b"hello", false, true, false), b"hello"); } #[test] diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index d8e4abd33ae..dc4c8e7b5ed 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -552,10 +552,14 @@ case class CometScanRule(session: SparkSession) fallbackReasons += s"Unsupported filesystem schemes: ${rootIssues.unsupportedSchemes.mkString(", ")}" } - if (rootIssues.multiBucket.nonEmpty) { - fallbackReasons += "Native Text scan reads S3-compliant alias paths across multiple " + - s"buckets (${rootIssues.multiBucket.toSeq.sorted.mkString(", ")}); Comet registers " + - "one object store per file partition and would read every file from the first bucket" + // Unlike the Parquet gate (which scopes this to alias scans to avoid regressing scans that + // work by luck), the Text scan is new, so decline any scan whose roots span more than one + // scheme/bucket -- a plain `s3a://a` + `s3a://b` read would otherwise read every file from + // the first bucket. + if (rootIssues.crossLocations.size > 1) { + fallbackReasons += "Native Text scan reads paths across multiple schemes/buckets " + + s"(${rootIssues.crossLocations.toSeq.sorted.mkString(", ")}); Comet registers one " + + "object store per file partition and would read every file from the first location" } rootIssues.rejectedPath.foreach { uri => fallbackReasons += s"Native Text scan cannot open path '$uri': " + @@ -1376,12 +1380,16 @@ object CometScanRule extends Logging { * - `multiBucket`: the S3-compliant alias buckets when a scan spans more than one; native * planning registers one object store per FilePartition and would read every file from the * first file's bucket. Empty when a single bucket (or no alias) is used. + * - `crossLocations`: the distinct `scheme://bucket` (or `scheme://authority`) across all + * roots that have one. Size > 1 means the scan spans multiple schemes/buckets; the Text + * gate declines these (the Parquet gate uses the narrower `multiBucket` instead). * - `rejectedPath`: a root whose recognized scheme carries a path object_store rejects (e.g. * a newline -> `%0A`), which native execution cannot open. */ private[rules] case class RootPathIssues( unsupportedSchemes: Set[String], multiBucket: Set[String], + crossLocations: Set[String], rejectedPath: Option[URI]) private[rules] def checkObjectStoreRootPaths( @@ -1401,12 +1409,27 @@ object CometScanRule extends Logging { !root.isLibhdfs && !isNativelyReadableScheme(root.uri, s3CompliantSchemes)) }.toSet val buckets = aliasScanBuckets(roots) + // Distinct scheme://bucket (or scheme://authority) across roots that have a scheme and a + // location. Native planning registers one object store per partition keyed on the first file's + // location, so a scan spanning more than one of these reads later files from the first. Covers + // plain s3a://a + s3a://b and gs://, abfs://, ... (schemeless/local paths contribute nothing). + val crossLocations = roots.flatMap { root => + root.scheme.flatMap { scheme => + root.bucket + .orElse(Option(root.uri.getAuthority).filter(_.nonEmpty)) + .map(loc => s"$scheme://$loc") + } + }.toSet val rejectedPath = roots .find(root => root.scheme.isDefined && !root.isLibhdfs && !root.isAlias && !objectStoreAcceptsPath(root.uri)) .map(_.uri) - RootPathIssues(unsupportedSchemes, if (buckets.size > 1) buckets else Set.empty, rejectedPath) + RootPathIssues( + unsupportedSchemes, + if (buckets.size > 1) buckets else Set.empty, + crossLocations, + rejectedPath) } /** diff --git a/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala b/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala index 42f13120b80..25eec030e81 100644 --- a/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala +++ b/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala @@ -99,8 +99,10 @@ object CometCsvNativeScanExec extends CometOperatorSerde[CometBatchScanExec] { new CSVOptions(csvScan.options.asScala.toMap, columnPruning, timeZone) } val filePartitions = op.inputPartitions.map(_.asInstanceOf[FilePartition]) + // Build the Hadoop conf from the scan's read options (case preserved), matching Spark's + // CSVScan.createReaderFactory, so per-read settings like fs.s3a.endpoint reach native. val hadoopConf = - sessionState.newHadoopConfWithOptions(op.session.sparkContext.conf.getAll.toMap) + sessionState.newHadoopConfWithOptions(csvScan.options.asCaseSensitiveMap.asScala.toMap) val fileUris = filePartitions.view.flatMap(_.files).map(_.pathUri) val objectStoreOptions = NativeConfig.extractScanObjectStoreOptions(hadoopConf, fileUris) match { diff --git a/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala index ced8fa75310..f3874f8f4f3 100644 --- a/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala +++ b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala @@ -108,8 +108,11 @@ object CometTextNativeScanExec extends CometOperatorSerde[CometBatchScanExec] { val objectStoreOptions = filePartitions.headOption .flatMap { partitionFile => + // Build the Hadoop conf from the scan's read options (case preserved), matching Spark's + // TextScan.createReaderFactory. Using SparkConf alone would drop per-read settings like + // fs.s3a.endpoint and send the wrong endpoint to native execution. val hadoopConf = sessionState - .newHadoopConfWithOptions(op.session.sparkContext.conf.getAll.toMap) + .newHadoopConfWithOptions(textScan.options.asCaseSensitiveMap.asScala.toMap) partitionFile.files.headOption .map(file => NativeConfig.extractObjectStoreOptions(hadoopConf, file.pathUri)) } diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index c1bc243c5e6..7e96e925a8d 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -290,13 +290,46 @@ class CometTextNativeReadSuite extends CometTestBase { assume(isSpark41Plus, "Spark 4.1+ treats .gzip/.zstd as compressed") withTempDir { dir => val gzip = new File(dir, "data.txt.gzip") - val out = new java.util.zip.GZIPOutputStream(new java.io.FileOutputStream(gzip)) - out.write("a\nb\nc".getBytes(StandardCharsets.UTF_8)) - out.close() + val gzipOut = new java.util.zip.GZIPOutputStream(new java.io.FileOutputStream(gzip)) + gzipOut.write("a\nb\nc".getBytes(StandardCharsets.UTF_8)) + gzipOut.close() + val zstd = new File(dir, "data.txt.zstd") + val zstdOut = new com.github.luben.zstd.ZstdOutputStream(new java.io.FileOutputStream(zstd)) + zstdOut.write("a\nb\nc".getBytes(StandardCharsets.UTF_8)) + zstdOut.close() withTextConf { checkSparkAnswerAndFallbackReason( spark.read.text(gzip.getAbsolutePath), "Comet does not support compressed text files") + checkSparkAnswerAndFallbackReason( + spark.read.text(zstd.getAbsolutePath), + "Comet does not support compressed text files") + } + } + } + + test("native text read - custom lineSep equal to a BOM is not stripped") { + withTempDir { dir => + val file = new File(dir, "bom-linesep.txt") + val bom = "\uFEFF" + // With lineSep=BOM, Spark leaves the BOM as the separator: `BOM a BOM b` -> ["", "a", "b"]. + Files.write(file.toPath, s"${bom}a${bom}b".getBytes(StandardCharsets.UTF_8)) + withTextConf { + checkSparkAnswerAndOperator(spark.read.option("lineSep", bom).text(file.getAbsolutePath)) + } + } + } + + test("native text read - BOM-only file matches Spark for default and BOM separators") { + withTempDir { dir => + val f = new File(dir, "bom-only.txt") + Files.write(f.toPath, "\uFEFF".getBytes(StandardCharsets.UTF_8)) + withTextConf { + // Default separator: the BOM is stripped, leaving no rows. + checkSparkAnswerAndOperator(spark.read.text(f.getAbsolutePath)) + // lineSep=BOM: the BOM is the (trailing) separator, leaving one empty row. + checkSparkAnswerAndOperator( + spark.read.option("lineSep", "\uFEFF").text(f.getAbsolutePath)) } } } From 8dac218a9def3b4dcf0d6bbd3fd9ed954215b324 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Mon, 12 Oct 2026 13:41:48 -0700 Subject: [PATCH 05/12] fmt --- native/core/src/execution/operators/text_scan.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/native/core/src/execution/operators/text_scan.rs b/native/core/src/execution/operators/text_scan.rs index 60a00d1b3b9..7a20d8f8897 100644 --- a/native/core/src/execution/operators/text_scan.rs +++ b/native/core/src/execution/operators/text_scan.rs @@ -573,10 +573,17 @@ mod tests { ); // BOM-only file with lineSep=BOM -> one empty record (trailing separator dropped). assert_eq!( - lines_as_str(strip_leading_bom(&bom, false, true, true), false, Some(&bom)), + lines_as_str( + strip_leading_bom(&bom, false, true, true), + false, + Some(&bom) + ), vec![""] ); - assert_eq!(strip_leading_bom(&with_hello, true, true, false), &with_hello[..]); + assert_eq!( + strip_leading_bom(&with_hello, true, true, false), + &with_hello[..] + ); assert_eq!( strip_leading_bom(&with_hello, false, false, false), &with_hello[..] From 4f837dae4cac42810ad42d23485392e4a91d0ab1 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Tue, 13 Oct 2026 09:44:43 -0700 Subject: [PATCH 06/12] fix: use shared V2 input_file_name fallback message in text scan The Text branch had its own input_file_name/_block guard that emitted a Native Text scan ... message, but the blanket V2 gate at the top of transformV2Scan already handles these expressions and emits Native V2 scan ... first. The Text message was dead, and the test asserted it, so the scans suite failed on all five Spark profiles. Drop the redundant guard and assert the shared message. --- .../scala/org/apache/comet/rules/CometScanRule.scala | 11 ++--------- .../apache/comet/text/CometTextNativeReadSuite.scala | 5 ++--- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index dc4c8e7b5ed..24720926ce2 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -436,15 +436,8 @@ case class CometScanRule(session: SparkSession) scanExec, s"Native Text scan requires ${COMET_EXEC_ENABLED.key} to be enabled") } - // input_file_name/_block_start/_block_length read a thread-local that Spark's FileScanRDD - // sets; the native DataFusion scan does not, so they would return "" / -1. Reuse the shared - // helper the Parquet gate uses so the two cannot drift. - if (CometScanRule.readsInputFileBlock(plan)) { - return withFallbackReason( - scanExec, - "Native Text scan is not compatible with input_file_name, " + - "input_file_block_start, or input_file_block_length") - } + // input_file_name/_block_start/_block_length are already handled by the blanket V2 gate + // near the top of transformV2Scan, so no Text-specific check is needed here. val fallbackReasons = new ListBuffer[String]() val schemaSupported = CometBatchScanExec.isSchemaSupported(scan.readDataSchema, fallbackReasons) diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index 7e96e925a8d..61b948b7a22 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -82,9 +82,8 @@ class CometTextNativeReadSuite extends CometTestBase { test("native text read - fallback for input_file_name") { withTextConf { val df = spark.read.text(TEST_TEXT_PATH).withColumn("f", input_file_name()) - checkSparkAnswerAndFallbackReason( - df, - "Native Text scan is not compatible with input_file_name") + // The blanket V2 input_file_name gate (shared by all native V2 scans) emits this. + checkSparkAnswerAndFallbackReason(df, "is not compatible with input_file_name") } } From 7d5b46f60536b6c338a5a0e27c05f4d13d61bc5c Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Tue, 13 Oct 2026 10:19:38 -0700 Subject: [PATCH 07/12] fix: fall back for self-overlapping text line separators Spark splits a custom lineSep with Hadoop LineReader.readCustomLine, which backtracks only one byte on a mismatch instead of following the separator's own prefix overlap. For a separator that overlaps itself (e.g. aab, whose aa prefix can restart a match) that reader can split differently from the native reader's exact substring scan. Rather than reproduce Hadoop's buffered matcher, fall back at plan time when the separator self-overlaps (KMP prefix function has a nonzero entry); overlap-free separators stay native. Guarded to line mode since wholetext ignores the separator. Added a test with lineSep=aab. --- .../apache/comet/rules/CometScanRule.scala | 44 +++++++++++++++++++ .../comet/text/CometTextNativeReadSuite.scala | 15 +++++++ 2 files changed, 59 insertions(+) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index 24720926ce2..6951336f525 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -521,6 +521,21 @@ case class CometScanRule(session: SparkSession) val textOptions = new org.apache.spark.sql.execution.datasources.text.TextOptions( scan.options.asScala.toMap) + // Spark reads a custom line separator with Hadoop's LineReader.readCustomLine, which on a + // mismatch backtracks only one byte instead of following the separator's own prefix + // overlap. For a separator that overlaps itself (e.g. "aab", whose "aa" prefix can restart + // a match) that reader can split differently from the native reader's exact substring + // scan, and the difference depends on where the separator falls across Hadoop's input + // buffer boundaries. wholetext ignores the separator, so only guard line mode. Fall back + // so the two always agree. + if (!textOptions.wholeText) { + textOptions.lineSeparatorInRead.foreach { sep => + if (CometScanRule.separatorSelfOverlaps(sep)) { + fallbackReasons += "Comet native Text scan does not support a self-overlapping " + + "line separator (Spark's line reader can split it differently)" + } + } + } if (textOptions.wholeText) { val maxWholeTextBytes = math.min(session.sessionState.conf.filesMaxPartitionBytes, Int.MaxValue.toLong / 3) @@ -1310,6 +1325,35 @@ object CometScanRule extends Logging { case _ => false })) + /** + * True when a custom line separator overlaps itself -- some proper prefix of the separator is + * also a suffix of one of its prefixes, so a partial match can restart (e.g. "aab", or "aba"). + * Spark splits custom separators with Hadoop's LineReader.readCustomLine, which backtracks only + * one byte on a mismatch instead of following that overlap, so for such a separator it can + * split differently from the native reader's exact substring scan. Uses the KMP prefix + * function: any nonzero entry means the separator overlaps itself. + */ + private[rules] def separatorSelfOverlaps(sep: Array[Byte]): Boolean = { + val f = new Array[Int](sep.length) + var k = 0 + var overlaps = false + var i = 1 + while (i < sep.length) { + while (k > 0 && sep(i) != sep(k)) { + k = f(k - 1) + } + if (sep(i) == sep(k)) { + k += 1 + } + f(i) = k + if (k > 0) { + overlaps = true + } + i += 1 + } + overlaps + } + // Memo of `NativeBase.isObjectStoreSchemeSupported`, keyed by the probe URL rather than the // scheme: object_store's parser keys on (scheme, host-presence), so an authorityless URL would // otherwise poison the authority-bearing form of the same scheme. diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index 61b948b7a22..2c899b60475 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -319,6 +319,21 @@ class CometTextNativeReadSuite extends CometTestBase { } } + test("native text read - fallback for a self-overlapping custom lineSep") { + withTempDir { dir => + val file = new File(dir, "overlap.txt") + // "aab" overlaps itself (its "aa" prefix can restart a match). Hadoop's readCustomLine, which + // Spark uses, can split such input differently from the native reader's exact substring scan, + // so Comet must fall back. Spark's own result is the reference. + Files.write(file.toPath, "aaabz".getBytes(StandardCharsets.UTF_8)) + withTextConf { + checkSparkAnswerAndFallbackReason( + spark.read.option("lineSep", "aab").text(file.getAbsolutePath), + "self-overlapping line separator") + } + } + } + test("native text read - BOM-only file matches Spark for default and BOM separators") { withTempDir { dir => val f = new File(dir, "bom-only.txt") From fcb87df1d0370b174fb72623acf5a728f5b7693b Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Tue, 20 Oct 2026 13:24:04 -0700 Subject: [PATCH 08/12] fix: fall back to the wrapped Spark scan when native text conversion is declined When the Text serializer declines a scan (for example files spread over two ABFS containers), CometExecRule kept the CometBatchScanExec, which advertises columnar output while Spark's text reader produces UnsafeRow, so the query failed with a ClassCastException. Return the wrapped scan like the CSV branch does, and add Text tests for the ABFS decline layouts. --- .../apache/comet/rules/CometExecRule.scala | 3 +- .../objectstore/AbfsScanFallbackSuite.scala | 32 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala index 2e44dfa6901..f5668780329 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala @@ -458,8 +458,9 @@ case class CometExecRule(session: SparkSession, queryStagePrep: Boolean = false) case scan: CometBatchScanExec if scan.wrapped.scan.isInstanceOf[CSVScan] => convertToComet(scan, CometCsvNativeScanExec).getOrElse(scan.wrapped) + // A declined native Text scan falls back to Spark's scan, which produces rows. case scan: CometBatchScanExec if scan.wrapped.scan.isInstanceOf[TextScan] => - convertToComet(scan, CometTextNativeScanExec).getOrElse(scan) + convertToComet(scan, CometTextNativeScanExec).getOrElse(scan.wrapped) // Comet JVM + native scan for V1 and V2 case op if isCometScan(op) => diff --git a/spark/src/test/scala/org/apache/comet/objectstore/AbfsScanFallbackSuite.scala b/spark/src/test/scala/org/apache/comet/objectstore/AbfsScanFallbackSuite.scala index 016491d8af8..b0ac15a2399 100644 --- a/spark/src/test/scala/org/apache/comet/objectstore/AbfsScanFallbackSuite.scala +++ b/spark/src/test/scala/org/apache/comet/objectstore/AbfsScanFallbackSuite.scala @@ -80,6 +80,18 @@ class AbfsScanFallbackSuite extends CometTestBase { .write .csv(localDir(dir)) + private def writeText(dir: String, from: Int): Unit = + spark + .range(from.toLong, from + 10L) + .selectExpr("cast(id as string) as value") + .write + .text(localDir(dir)) + + // Text is left off the V1 source list so the scan plans as a V2 TextScan. + private val textConfs = Seq( + CometConf.COMET_TEXT_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "avro,csv,json,kafka,orc,parquet") + private val csvConfs = Seq(CometConf.COMET_CSV_V2_NATIVE_ENABLED.key -> "true", SQLConf.USE_V1_SOURCE_LIST.key -> "") @@ -146,4 +158,24 @@ class AbfsScanFallbackSuite extends CometTestBase { checkSparkAnswerAndFallbackReason(df, multipleAuthoritiesReason) } } + + // The serializer declines these layouts, so the scan must fall back to Spark's row-producing + // scan rather than stay a columnar CometBatchScanExec (which fails with a ClassCastException). + test("native Text scan over two accounts with different keys falls back to Spark") { + writeText("text-a", 0) + writeText("text-b", 100) + withSQLConf(textConfs ++ accountKeys("accta" -> "a2V5LWE=", "acctb" -> "a2V5LWI="): _*) { + val df = spark.read.text(abfss("data", "accta", "text-a"), abfss("data", "acctb", "text-b")) + checkSparkAnswerAndFallbackReason(df, multipleAuthoritiesReason) + } + } + + test("native Text scan over two containers of one account falls back to Spark") { + writeText("text-c", 0) + writeText("text-d", 100) + withSQLConf(textConfs ++ accountKeys("accta" -> "a2V5LWE="): _*) { + val df = spark.read.text(abfss("data", "accta", "text-c"), abfss("logs", "accta", "text-d")) + checkSparkAnswerAndFallbackReason(df, multipleAuthoritiesReason) + } + } } From b04673de2177499c1a61c8d3ed130acba25ded22 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Thu, 22 Oct 2026 10:05:01 -0700 Subject: [PATCH 09/12] fix: strip the leading BOM from the first record for custom line separators too Hadoop's LineRecordReader reads the first record and then drops a leading UTF-8 BOM from it, whatever the separator. The earlier fix skipped the strip for every custom separator, so a file like BOM a|b with lineSep=| returned BOM+a, and a BOM-only file returned one row instead of none. Split first, then strip the BOM from the first record only. A file that is only the BOM has no rows; BOM followed by a separator leaves an empty row; lineSep equal to the BOM is unaffected because the BOM is then the separator. Add Rust unit tests and Scala tests for BOM a|b, BOM|x, and a BOM-only file with lineSep=|. --- .../core/src/execution/operators/text_scan.rs | 122 ++++++++++++------ .../comet/text/CometTextNativeReadSuite.scala | 27 ++++ 2 files changed, 108 insertions(+), 41 deletions(-) diff --git a/native/core/src/execution/operators/text_scan.rs b/native/core/src/execution/operators/text_scan.rs index 7a20d8f8897..c82130772f4 100644 --- a/native/core/src/execution/operators/text_scan.rs +++ b/native/core/src/execution/operators/text_scan.rs @@ -250,14 +250,14 @@ impl FileOpener for TextOpener { }; let mut timer = baseline_metrics.elapsed_compute().timer(); - // Spark's line reader strips a leading UTF-8 BOM at the start of the file, but only with - // the default separator. With a custom lineSep the BOM is ordinary data (Spark leaves it - // for the splitter, so e.g. a file `BOM a BOM b` read with lineSep=BOM yields - // ["", "a", "b"]). Only strip in line mode, at file offset 0 (native reads unsplit files - // only), and when no custom lineSep is set. wholetext keeps the BOM. + // Spark's line reader (Hadoop LineRecordReader.skipUtfByteOrderMark) strips a leading + // UTF-8 BOM from the first record at file offset 0, whatever the separator. It does so + // after finding the first record, so split first and then strip. Native reads unsplit + // files only. wholetext keeps the BOM. let at_file_start = range.as_ref().map(|r| r.start == 0).unwrap_or(true); - let content = strip_leading_bom(&buf, whole_text, at_file_start, line_sep.is_some()); + let content: &[u8] = &buf; let mut lines = split_lines(content, whole_text, line_sep.as_deref()); + strip_first_record_bom(&mut lines, content.len(), whole_text, at_file_start); // Spark may split an uncompressed file into byte ranges (TextScan.isSplitable). A line // belongs to the split whose [start, end) contains the line's start offset -- so each // line is emitted by exactly one split, with no duplication or gaps across splits. See @@ -275,20 +275,31 @@ impl FileOpener for TextOpener { } } -/// Strip a single leading UTF-8 BOM the way Spark's default line reader does: only in line mode -/// (not wholetext), only for the split that starts at file offset 0, and only with the default -/// separator. With a custom lineSep the BOM is left in place as ordinary data. -fn strip_leading_bom( - buf: &[u8], +/// Strip a leading UTF-8 BOM from the first record the way Hadoop's LineRecordReader does: only +/// in line mode (not wholetext), only for the split that starts at file offset 0, and from the +/// first record only. Hadoop reads the first record, then drops the BOM from its text. When that +/// leaves nothing and the record was not terminated (the file is only the BOM), it counts as end +/// of input, so the file has no rows. A BOM followed by a separator leaves an empty row. A custom +/// `lineSep` equal to the BOM never reaches the strip, because the BOM is then the separator and +/// the first record does not start with it. +fn strip_first_record_bom<'a>( + lines: &mut Vec<&'a [u8]>, + file_len: usize, whole_text: bool, at_file_start: bool, - custom_line_sep: bool, -) -> &[u8] { - if !whole_text && at_file_start && !custom_line_sep && buf.starts_with(UTF8_BOM) { - &buf[UTF8_BOM.len()..] - } else { - buf +) { + if whole_text || !at_file_start { + return; + } + if !lines.first().is_some_and(|l| l.starts_with(UTF8_BOM)) { + return; } + if file_len == UTF8_BOM.len() { + lines.clear(); + return; + } + let record: &'a [u8] = lines[0]; + lines[0] = &record[UTF8_BOM.len()..]; } /// Split a decompressed file buffer into text lines following Spark's semantics. @@ -546,49 +557,78 @@ mod tests { assert_eq!(batches[0].num_rows(), 3); } + /// Split, then strip the first-record BOM, as the opener does. + fn lines_after_bom( + buf: &[u8], + whole_text: bool, + at_file_start: bool, + line_sep: Option<&[u8]>, + ) -> Vec { + let mut lines = split_lines(buf, whole_text, line_sep); + strip_first_record_bom(&mut lines, buf.len(), whole_text, at_file_start); + lines + .iter() + .map(|l| String::from_utf8_lossy(l).into_owned()) + .collect() + } + #[test] - fn strips_leading_utf8_bom_in_line_mode() { + fn strips_first_record_bom_in_line_mode() { let bom = [0xEFu8, 0xBB, 0xBF]; let with_hello = [&bom[..], b"hello"].concat(); // Default separator at file start: the BOM is stripped. - assert_eq!(strip_leading_bom(&with_hello, false, true, false), b"hello"); - // BOM then newline -> ["", "hello"] after split. + assert_eq!( + lines_after_bom(&with_hello, false, true, None), + vec!["hello"] + ); + // BOM then newline -> ["", "hello"]. let bom_nl = [&bom[..], b"\nhello"].concat(); assert_eq!( - lines_as_str(strip_leading_bom(&bom_nl, false, true, false), false, None), + lines_after_bom(&bom_nl, false, true, None), vec!["", "hello"] ); - // BOM-only file, default separator -> empty after strip -> zero lines. - assert!(split_lines(strip_leading_bom(&bom, false, true, false), false, None).is_empty()); - // NOT stripped: a custom lineSep (BOM is data), so `BOM a BOM b` split on BOM is - // ["", "a", "b"]; wholetext; a non-start split; or no BOM present. + // BOM-only file, default separator -> no rows. + assert!(lines_after_bom(&bom, false, true, None).is_empty()); + // BOM only plus its terminator -> one empty row. + let bom_only_nl = [&bom[..], b"\n"].concat(); + assert_eq!(lines_after_bom(&bom_only_nl, false, true, None), vec![""]); + // Custom separator: the BOM is still stripped from the first record. + let bom_a_b = [&bom[..], b"a|b"].concat(); + assert_eq!( + lines_after_bom(&bom_a_b, false, true, Some(b"|")), + vec!["a", "b"] + ); + // BOM-only file with a custom separator -> no rows. + assert!(lines_after_bom(&bom, false, true, Some(b"|")).is_empty()); + // BOM then separator -> an empty row, then the rest. + let bom_sep_x = [&bom[..], b"|x"].concat(); + assert_eq!( + lines_after_bom(&bom_sep_x, false, true, Some(b"|")), + vec!["", "x"] + ); + // lineSep equal to the BOM: the BOM is the separator, so the first record does not start + // with it and nothing is stripped. `BOM a BOM b` -> ["", "a", "b"]. let bom_a_bom_b = [&bom[..], b"a", &bom[..], b"b"].concat(); assert_eq!( - lines_as_str( - strip_leading_bom(&bom_a_bom_b, false, true, true), - false, - Some(&bom) - ), + lines_after_bom(&bom_a_bom_b, false, true, Some(&bom)), vec!["", "a", "b"] ); // BOM-only file with lineSep=BOM -> one empty record (trailing separator dropped). + assert_eq!(lines_after_bom(&bom, false, true, Some(&bom)), vec![""]); + // Not stripped: wholetext, a split that does not start at offset 0, or no BOM present. assert_eq!( - lines_as_str( - strip_leading_bom(&bom, false, true, true), - false, - Some(&bom) - ), - vec![""] - ); - assert_eq!( - strip_leading_bom(&with_hello, true, true, false), + lines_after_bom(&with_hello, true, true, None) + .concat() + .as_bytes(), &with_hello[..] ); assert_eq!( - strip_leading_bom(&with_hello, false, false, false), + lines_after_bom(&with_hello, false, false, None) + .concat() + .as_bytes(), &with_hello[..] ); - assert_eq!(strip_leading_bom(b"hello", false, true, false), b"hello"); + assert_eq!(lines_after_bom(b"hello", false, true, None), vec!["hello"]); } #[test] diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index 2c899b60475..21c5c4a4981 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -348,6 +348,33 @@ class CometTextNativeReadSuite extends CometTestBase { } } + // Hadoop's LineRecordReader strips a leading BOM from the first record whatever the separator. + test("native text read - custom lineSep still strips a leading BOM from the first record") { + withTempDir { dir => + val bom = "\uFEFF" + withTextConf { + // `BOM a|b` -> ["a", "b"] + val ab = new File(dir, "bom-a-b.txt") + Files.write(ab.toPath, s"${bom}a|b".getBytes(StandardCharsets.UTF_8)) + checkSparkAnswerAndOperator(spark.read.option("lineSep", "|").text(ab.getAbsolutePath)) + // `BOM|x` -> ["", "x"] + val bx = new File(dir, "bom-sep-x.txt") + Files.write(bx.toPath, s"$bom|x".getBytes(StandardCharsets.UTF_8)) + checkSparkAnswerAndOperator(spark.read.option("lineSep", "|").text(bx.getAbsolutePath)) + } + } + } + + test("native text read - BOM-only file with a custom lineSep has no rows") { + withTempDir { dir => + val f = new File(dir, "bom-only-pipe.txt") + Files.write(f.toPath, "\uFEFF".getBytes(StandardCharsets.UTF_8)) + withTextConf { + checkSparkAnswerAndOperator(spark.read.option("lineSep", "|").text(f.getAbsolutePath)) + } + } + } + test("native text read - fallback for line.maxlength") { withTempDir { dir => val file = new File(dir, "lines.txt") From bd3a1c1559d15e1fc90635b926aef8a343843cfc Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Thu, 22 Oct 2026 16:16:48 -0700 Subject: [PATCH 10/12] fix: build the text gate's Hadoop conf from the case-preserving option map The Text eligibility gate built its Hadoop conf from scan.options.asScala, which lowercases keys. Hadoop keys are case sensitive and Spark's TextScan keeps their case, so a differently cased read option such as ...line.maxLength did not override the Hadoop conf limit in Spark, but the gate saw the option as the lowercase key, read an unlimited length, and admitted native execution, which returned the long line Spark skips. Use asCaseSensitiveMap, matching TextScan.createReaderFactory and the serializer, and add a test. --- .../apache/comet/rules/CometScanRule.scala | 7 +++-- .../comet/text/CometTextNativeReadSuite.scala | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index d6c8b20781a..5988d194780 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -477,10 +477,13 @@ case class CometScanRule(session: SparkSession) } // Comet's native plan does not carry a compression codec, so the native reader would feed // raw compressed bytes to the line splitter. Spark auto-detects a codec by file extension. - // Use the options-aware Hadoop conf (so a per-read codec option is honored) and the + // Use the options-aware Hadoop conf, built from the case-preserving option map like + // Spark's TextScan.createReaderFactory (Hadoop keys are case sensitive, but the scan's + // option map lowercases them), so a per-read codec option is honored, and the // version-aware ShimFileFormat.isCompressedFile, which on Spark 4.1+ also catches the // non-standard .gzip/.zstd extensions that CompressionCodecFactory alone misses. - val hadoopConf = session.sessionState.newHadoopConfWithOptions(scan.options.asScala.toMap) + val hadoopConf = session.sessionState.newHadoopConfWithOptions( + scan.options.asCaseSensitiveMap.asScala.toMap) val hasCompressedFile = scan.fileIndex.inputFiles.exists { path => ShimFileFormat.isCompressedFile(hadoopConf, new org.apache.hadoop.fs.Path(path)) } diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index 21c5c4a4981..e39cfc4c5b6 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -391,6 +391,32 @@ class CometTextNativeReadSuite extends CometTestBase { } } + test( + "native text read - line.maxlength from the Hadoop conf is not overridden by a differently " + + "cased read option") { + withTempDir { dir => + val file = new File(dir, "lines-case.txt") + Files.write(file.toPath, "ab\nabcdefghij\nxy".getBytes(StandardCharsets.UTF_8)) + val key = "mapreduce.input.linerecordreader.line.maxlength" + val hadoopConf = spark.sparkContext.hadoopConfiguration + hadoopConf.set(key, "5") + try { + withTextConf { + // Hadoop keys are case sensitive, so this option does not override the limit of 5 and + // Spark skips the long line. The gate must see the option with its case preserved, or it + // reads an unlimited length and the native scan returns the long line too. + checkSparkAnswerAndFallbackReason( + spark.read + .option("mapreduce.input.linerecordreader.line.maxLength", "2147483647") + .text(file.getAbsolutePath), + key) + } + } finally { + hadoopConf.unset(key) + } + } + } + test("native text read - fallback for files larger than the size ceiling") { withTempDir { dir => val file = new File(dir, "big.txt") From 051b9c886b7cb4d6140d1af7299271bf09b7f420 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Sun, 25 Oct 2026 14:37:22 -0700 Subject: [PATCH 11/12] fix: address text scan review findings on file selection, gates, metrics - Resolve the codec and check files on the selected partitions' decoded paths, not fileIndex.inputFiles (URL-encoded, and including pruned files). - Fall back for local files that have a Hadoop .crc checksum file. - Run the scheme and object_store path checks on the selected files, not only the roots. - Fall back for mapreduce.job.input.file.must.* options in line mode. - Read line.maxlength only in line mode, and fall back instead of failing when it is not an integer. - Decline CSV scans whose libhdfs scheme is set only by a read option. - Canonicalize the Text scan's output and wrapped scan so repeated lookups compare equal. - Report bytes_scanned from the native opener and expose it with output_rows so the task input metrics are filled in. --- docs/source/user-guide/latest/datasources.md | 4 +- .../core/src/execution/operators/text_scan.rs | 6 +- .../comet/objectstore/NativeConfig.scala | 8 +- .../apache/comet/rules/CometScanRule.scala | 121 +++++++---- .../sql/comet/CometCsvNativeScanExec.scala | 21 +- .../sql/comet/CometTextNativeScanExec.scala | 24 ++- .../comet/csv/CometCsvNativeReadSuite.scala | 21 ++ .../comet/text/CometTextNativeReadSuite.scala | 194 +++++++++++++++++- 8 files changed, 353 insertions(+), 46 deletions(-) diff --git a/docs/source/user-guide/latest/datasources.md b/docs/source/user-guide/latest/datasources.md index b3a6610e622..1768b36c0ae 100644 --- a/docs/source/user-guide/latest/datasources.md +++ b/docs/source/user-guide/latest/datasources.md @@ -58,7 +58,9 @@ text through the V1 API by default, so also remove `text` from `spark.sql.source falls back to Spark for cases it does not handle -- including compressed files, files larger than `spark.comet.scan.text.maxFileSize` (64MB by default, since the reader loads each file whole), files Spark splits into byte ranges, unsupported filesystem schemes, and `input_file_name()` or -`ignoreCorruptFiles`/`ignoreMissingFiles` usage. It targets small lookup tables such as a broadcast join's build side. +`ignoreCorruptFiles`/`ignoreMissingFiles` usage. It also falls back for local files that have a Hadoop checksum +(`.crc`) file, such as data Spark itself wrote to the local filesystem, because Hadoop verifies the checksum on read and +the native reader does not. It targets small lookup tables such as a broadcast join's build side. Reading a partition column from a partitioned directory falls back to Spark, but selecting only `value` stays native. Text files are a common source of ill-formed UTF-8. The native reader replaces invalid byte sequences with the diff --git a/native/core/src/execution/operators/text_scan.rs b/native/core/src/execution/operators/text_scan.rs index c82130772f4..805802b3559 100644 --- a/native/core/src/execution/operators/text_scan.rs +++ b/native/core/src/execution/operators/text_scan.rs @@ -30,7 +30,7 @@ use arrow::record_batch::{RecordBatch, RecordBatchOptions}; use datafusion::common::Result; use datafusion::datasource::object_store::ObjectStoreUrl; use datafusion::physical_expr::projection::ProjectionExprs; -use datafusion::physical_plan::metrics::{BaselineMetrics, ExecutionPlanMetricsSet}; +use datafusion::physical_plan::metrics::{BaselineMetrics, ExecutionPlanMetricsSet, MetricBuilder}; use datafusion::physical_plan::DisplayFormatType; use datafusion_comet_common::decode_utf8_spark_lossy; use datafusion_comet_proto::spark_operator::TextOptions; @@ -224,11 +224,15 @@ impl FileOpener for TextOpener { let whole_text = self.whole_text; let line_sep = self.line_sep.clone(); let baseline_metrics = BaselineMetrics::new(&self.metrics, self.partition_index); + // Feeds the task's input bytes in the Spark UI (see CometMetricNode.reportScanInputMetrics). + let bytes_scanned = + MetricBuilder::new(&self.metrics).counter("bytes_scanned", self.partition_index); Ok(Box::pin(async move { let range = partitioned_file.range.clone(); let location = partitioned_file.object_meta.location; let raw = store.get(&location).await?.bytes().await?; + bytes_scanned.add(raw.len()); // Decompress only when the file is actually compressed. On Comet's plan config the file // is always UNCOMPRESSED (CometScanRule falls back for compressed files), so we borrow diff --git a/spark/src/main/scala/org/apache/comet/objectstore/NativeConfig.scala b/spark/src/main/scala/org/apache/comet/objectstore/NativeConfig.scala index 0cdf8183bfe..575dadddb62 100644 --- a/spark/src/main/scala/org/apache/comet/objectstore/NativeConfig.scala +++ b/spark/src/main/scala/org/apache/comet/objectstore/NativeConfig.scala @@ -74,7 +74,7 @@ object NativeConfig { s3Properties.map { case (_, s3aSuffix, icebergKey) => s3aSuffix -> icebergKey }.toMap /** A URI's scheme, lowercased; None when it has none. */ - private[comet] def lowerScheme(uri: URI): Option[String] = + def lowerScheme(uri: URI): Option[String] = Option(uri.getScheme).map(_.toLowerCase(Locale.ROOT)) // Comma-separated scheme list -> trimmed, lowercased set (case-insensitive). Shared with @@ -146,7 +146,7 @@ object NativeConfig { * which native reads through libhdfs. `scanOnly` holds those listed only through the scan's * reader options, which the scan gates decline. */ - private[comet] case class ScanLibhdfsSchemes(libhdfs: Set[String], scanOnly: Set[String]) + case class ScanLibhdfsSchemes(libhdfs: Set[String], scanOnly: Set[String]) /** * Splits the `fs.comet.libhdfs.schemes` lists of `sessionConf` (the session's Hadoop conf) and @@ -154,7 +154,7 @@ object NativeConfig { * the scan's list, but libhdfs builds its configuration from the executor classpath * (`core-site.xml`), not the scan's options, so a reader option cannot add a libhdfs scheme. */ - private[comet] def resolveScanLibhdfsSchemes( + def resolveScanLibhdfsSchemes( sessionConf: Configuration, scanConf: Configuration): ScanLibhdfsSchemes = resolveScanLibhdfsSchemes(resolveLibhdfsSchemes(sessionConf), scanConf) @@ -163,7 +163,7 @@ object NativeConfig { * As above, with the session's list already resolved by [[resolveLibhdfsSchemes]], so a caller * planning many scans copies the session's Hadoop conf once. */ - private[comet] def resolveScanLibhdfsSchemes( + def resolveScanLibhdfsSchemes( sessionSchemes: Set[String], scanConf: Configuration): ScanLibhdfsSchemes = { val scan = resolveLibhdfsSchemes(scanConf) diff --git a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala index 5988d194780..fb18b697ca3 100644 --- a/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala +++ b/spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala @@ -475,21 +475,39 @@ case class CometScanRule(session: SparkSession) fallbackReasons += "Comet native Text scan does not read files larger than " + s"${COMET_SCAN_TEXT_MAX_FILE_SIZE.key} ($maxTextFileSize bytes)" } - // Comet's native plan does not carry a compression codec, so the native reader would feed - // raw compressed bytes to the line splitter. Spark auto-detects a codec by file extension. - // Use the options-aware Hadoop conf, built from the case-preserving option map like + // Build the Hadoop conf from the scan's read options with their case preserved, like // Spark's TextScan.createReaderFactory (Hadoop keys are case sensitive, but the scan's - // option map lowercases them), so a per-read codec option is honored, and the - // version-aware ShimFileFormat.isCompressedFile, which on Spark 4.1+ also catches the - // non-standard .gzip/.zstd extensions that CompressionCodecFactory alone misses. + // option map lowercases them). val hadoopConf = session.sessionState.newHadoopConfWithOptions( scan.options.asCaseSensitiveMap.asScala.toMap) - val hasCompressedFile = scan.fileIndex.inputFiles.exists { path => - ShimFileFormat.isCompressedFile(hadoopConf, new org.apache.hadoop.fs.Path(path)) + val textOptions = + new org.apache.spark.sql.execution.datasources.text.TextOptions( + scan.options.asScala.toMap) + // The files this scan reads. `fileIndex.inputFiles` also lists files that partition + // pruning dropped, so the per-file checks below use the selected partitions instead. + val selectedFiles = scanExec.inputPartitions.flatMap { + case fp: FilePartition => fp.files.toSeq + case _ => Seq.empty + } + // Comet's native plan does not carry a compression codec, so the native reader would feed + // raw compressed bytes to the line splitter. Spark auto-detects a codec by file extension, + // from the decoded path (`Path(URI)`), so a name like `a b.gz` keeps its suffix. The + // version-aware ShimFileFormat.isCompressedFile also catches the non-standard .gzip/.zstd + // extensions on Spark 4.1+ that CompressionCodecFactory alone misses. + val hasCompressedFile = selectedFiles.exists { file => + ShimFileFormat.isCompressedFile(hadoopConf, new org.apache.hadoop.fs.Path(file.pathUri)) } if (hasCompressedFile) { fallbackReasons += "Comet does not support compressed text files" } + // Hadoop's local filesystem verifies a `..crc` checksum file when one exists and + // raises a ChecksumException if the data no longer matches it. The native reader reads the + // file directly and would return the altered rows, so fall back for such files. + if (selectedFiles.exists(file => + CometScanRule.hasLocalChecksumFile(hadoopConf, file.pathUri))) { + fallbackReasons += "Comet native Text scan does not support local files that have a " + + "Hadoop checksum (.crc) file" + } // Spark skips unreadable/missing files when these are set; the native reader's DataFusion // FileStream fails the whole scan instead (OnError::Fail). Fall back to match Spark. if (SQLConf.get.ignoreCorruptFiles || scan.options.getBoolean( @@ -502,32 +520,36 @@ case class CometScanRule(session: SparkSession) false)) { fallbackReasons += "Comet native Text scan does not support ignoreMissingFiles" } - // Spark's line reader skips any line at or over - // mapreduce.input.linerecordreader.line.maxlength (default unlimited); the key can come - // from spark.hadoop.*, core-site.xml, or a read option. The native reader does not - // truncate, so fall back when a finite limit is set. - if (hadoopConf.getInt("mapreduce.input.linerecordreader.line.maxlength", Int.MaxValue) - != Int.MaxValue) { - fallbackReasons += "Comet native Text scan does not support " + - "mapreduce.input.linerecordreader.line.maxlength" - } - // A wholetext file is never split (TextScan.isSplitable is false for wholeText), so the - // split-file guard above does not bound it. Reading a very large file whole is unbounded - // memory, and the whole file becomes one Arrow string value. Because a lossy UTF-8 decode - // can expand ill-formed bytes up to 3x, cap the raw file size at Int.MaxValue/3 so the - // decoded value stays under Arrow's 2GB 32-bit string-offset limit; fall back above that. - // (The native reader also fails cleanly rather than panicking if a value still overflows.) - val textOptions = - new org.apache.spark.sql.execution.datasources.text.TextOptions( - scan.options.asScala.toMap) - // Spark reads a custom line separator with Hadoop's LineReader.readCustomLine, which on a - // mismatch backtracks only one byte instead of following the separator's own prefix - // overlap. For a separator that overlaps itself (e.g. "aab", whose "aa" prefix can restart - // a match) that reader can split differently from the native reader's exact substring - // scan, and the difference depends on where the separator falls across Hadoop's input - // buffer boundaries. wholetext ignores the separator, so only guard line mode. Fall back - // so the two always agree. + // Only Spark's line reader consults the settings below. The wholetext reader does not, + // so a value it never reads must not fail or fall back a wholetext query. if (!textOptions.wholeText) { + // Spark's line reader skips any line at or over + // mapreduce.input.linerecordreader.line.maxlength (default unlimited); the key can come + // from spark.hadoop.*, core-site.xml, or a read option. The native reader does not + // truncate, so fall back when a finite limit is set. A value that is not an integer + // fails Spark's reader, so fall back and let Spark report it. + val maxLineLength = scala.util.Try( + hadoopConf.getInt("mapreduce.input.linerecordreader.line.maxlength", Int.MaxValue)) + if (maxLineLength.map(_ != Int.MaxValue).getOrElse(true)) { + fallbackReasons += "Comet native Text scan does not support " + + "mapreduce.input.linerecordreader.line.maxlength" + } + // Spark passes `mapreduce.job.input.file.must.*` to the filesystem's open call as + // mandatory options, and the read fails if the filesystem cannot honor one. The native + // reader opens files itself and cannot enforce them, so fall back and let Spark decide. + if (hadoopConf + .iterator() + .asScala + .exists(_.getKey.startsWith("mapreduce.job.input.file.must."))) { + fallbackReasons += "Comet native Text scan does not support mandatory file-open " + + "options (mapreduce.job.input.file.must.*)" + } + // Spark reads a custom line separator with Hadoop's LineReader.readCustomLine, which on + // a mismatch backtracks only one byte instead of following the separator's own prefix + // overlap. For a separator that overlaps itself (e.g. "aab", whose "aa" prefix can + // restart a match) that reader can split differently from the native reader's exact + // substring scan, and the difference depends on where the separator falls across + // Hadoop's input buffer boundaries. Fall back so the two always agree. textOptions.lineSeparatorInRead.foreach { sep => if (CometScanRule.separatorSelfOverlaps(sep)) { fallbackReasons += "Comet native Text scan does not support a self-overlapping " + @@ -535,6 +557,12 @@ case class CometScanRule(session: SparkSession) } } } + // A wholetext file is never split (TextScan.isSplitable is false for wholeText), so the + // split-file guard above does not bound it. Reading a very large file whole is unbounded + // memory, and the whole file becomes one Arrow string value. Because a lossy UTF-8 decode + // can expand ill-formed bytes up to 3x, cap the raw file size at Int.MaxValue/3 so the + // decoded value stays under Arrow's 2GB 32-bit string-offset limit; fall back above that. + // (The native reader also fails cleanly rather than panicking if a value still overflows.) if (textOptions.wholeText) { val maxWholeTextBytes = math.min(session.sessionState.conf.filesMaxPartitionBytes, Int.MaxValue.toLong / 3) @@ -551,11 +579,14 @@ case class CometScanRule(session: SparkSession) // set of URL schemes. A text file on a custom Hadoop scheme (viewfs://, oss://, ...) or a // path object_store rejects would be claimed here then hard-fail at execution, whereas // Spark reads it via the Hadoop FS API. Use the same shared gate as the native Parquet - // path. A scan that spans several object stores is split per store in the serde. + // path, on the selected files rather than the root paths: a legal Hadoop filename inside + // a directory (for example one with a tab in its name) can pass for its parent and still + // be rejected by object_store. A scan that spans several object stores is split per store + // in the serde. CometScanRule .rootPathFallbackReason( "Native Text scan", - scan.fileIndex.rootPaths.map(_.toUri), + selectedFiles.map(_.pathUri).distinct, NativeConfig.resolveLibhdfsSchemes(session.sessionState.newHadoopConf()), hadoopConf, isBucketedScan = false) @@ -1434,6 +1465,26 @@ object CometScanRule extends Logging { isAlias = scheme.exists(s3CompliantSchemes.contains)) } + /** + * True when a local file has a Hadoop checksum file (`..crc`) next to it. Hadoop's local + * filesystem verifies the data against that file on every read, which the native reader does + * not. Only local paths are checked; other filesystems are not asked, because creating one is + * costly. If the filesystem cannot be asked, assume a checksum file exists. + */ + private[rules] def hasLocalChecksumFile(hadoopConf: Configuration, uri: URI): Boolean = { + val isLocal = Option(uri.getScheme).forall(_.equalsIgnoreCase("file")) + isLocal && scala.util + .Try { + val path = new org.apache.hadoop.fs.Path(uri) + path.getFileSystem(hadoopConf) match { + case fs: org.apache.hadoop.fs.ChecksumFileSystem => + fs.getRawFileSystem.exists(fs.getChecksumFile(path)) + case _ => false + } + } + .getOrElse(true) + } + /** * Why a native scan of `rootUris` must fall back because of the filesystem scheme or path, or * None. A custom Hadoop FileSystem (e.g. registered via spark.hadoop.fs..impl) would diff --git a/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala b/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala index 29b50a29b51..b2c5a499010 100644 --- a/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala +++ b/spark/src/main/scala/org/apache/spark/sql/comet/CometCsvNativeScanExec.scala @@ -102,8 +102,27 @@ object CometCsvNativeScanExec extends CometOperatorSerde[CometBatchScanExec] { val hadoopConf = sessionState.newHadoopConfWithOptions(csvScan.options.asCaseSensitiveMap.asScala.toMap) val s3CompliantSchemes = NativeConfig.resolveS3CompliantSchemes(hadoopConf) - val libhdfsSchemes = NativeConfig.resolveLibhdfsSchemes(hadoopConf) + // A read option can list a scheme for libhdfs that the session does not. Native reads through + // libhdfs with the executor classpath's configuration, not the read options, so a scheme + // that only the options route there cannot be read natively. Decline those, as the native + // Parquet scan does. + val scanLibhdfs = NativeConfig.resolveScanLibhdfsSchemes( + NativeConfig.resolveLibhdfsSchemes(sessionState.newHadoopConf()), + hadoopConf) + val libhdfsSchemes = scanLibhdfs.libhdfs val inputPartitions = op.inputPartitions.map(_.asInstanceOf[FilePartition]) + val scanOnlyLibhdfsSchemes = inputPartitions.view + .flatMap(_.files.view.flatMap(file => NativeConfig.lowerScheme(file.pathUri))) + .filter(scanLibhdfs.scanOnly.contains) + .toSet + if (scanOnlyLibhdfsSchemes.nonEmpty) { + val reason = "Native CSV scan does not support filesystem scheme(s) " + + s"${scanOnlyLibhdfsSchemes.toSeq.sorted.mkString(", ")} listed for libhdfs only " + + "through reader options" + withFallbackReason(op, reason) + withFallbackReason(op.wrapped, reason) + return None + } val multiStoreReason = CometScanUtils.multiStoreFallbackReason( "Native CSV scan", inputPartitions.view.flatMap(_.files.view.map(_.pathUri)), diff --git a/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala index 7e63e347f26..0d687bf838b 100644 --- a/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala +++ b/spark/src/main/scala/org/apache/spark/sql/comet/CometTextNativeScanExec.scala @@ -22,12 +22,14 @@ package org.apache.spark.sql.comet import scala.jdk.CollectionConverters._ import org.apache.spark.sql.catalyst.expressions.{Attribute, SortOrder} +import org.apache.spark.sql.catalyst.plans.QueryPlan import org.apache.spark.sql.catalyst.plans.physical.{Partitioning, UnknownPartitioning} import org.apache.spark.sql.execution.SparkPlan import org.apache.spark.sql.execution.datasources.FilePartition import org.apache.spark.sql.execution.datasources.text.TextOptions import org.apache.spark.sql.execution.datasources.v2.BatchScanExec import org.apache.spark.sql.execution.datasources.v2.text.TextScan +import org.apache.spark.sql.execution.metric.{SQLMetric, SQLMetrics} import com.google.common.base.Objects @@ -63,8 +65,21 @@ case class CometTextNativeScanExec( override def outputOrdering: Seq[SortOrder] = Nil + // The scan reports the bytes it fetched as `bytes_scanned`, which CometMetricNode turns into the + // task's input bytes and records, so they show on the stage UI like any other scan. + override lazy val metrics: Map[String, SQLMetric] = + CometMetricNode.baselineMetrics(sparkContext) + + ("bytes_scanned" -> SQLMetrics.createSizeMetric(sparkContext, "number of bytes scanned")) + + // Two scans of the same files must compare equal so Spark reuses their exchanges and stages. + // That needs normalized output attribute ids and the wrapped scan's canonical form, and no + // serialization state, which differs between otherwise identical copies. override protected def doCanonicalize(): SparkPlan = { - CometTextNativeScanExec(nativeOp, output, originalPlan, serializedPlanOpt) + CometTextNativeScanExec( + nativeOp, + output.map(QueryPlan.normalizeExpressions(_, output)), + if (originalPlan == null) null else originalPlan.doCanonicalize(), + SerializedPlan(None)) } override def equals(obj: Any): Boolean = { @@ -101,7 +116,12 @@ object CometTextNativeScanExec extends CometOperatorSerde[CometBatchScanExec] { val hadoopConf = sessionState.newHadoopConfWithOptions(textScan.options.asCaseSensitiveMap.asScala.toMap) val s3CompliantSchemes = NativeConfig.resolveS3CompliantSchemes(hadoopConf) - val libhdfsSchemes = NativeConfig.resolveLibhdfsSchemes(hadoopConf) + // The scan gate already declined schemes that only the read options route through libhdfs. + val libhdfsSchemes = NativeConfig + .resolveScanLibhdfsSchemes( + NativeConfig.resolveLibhdfsSchemes(sessionState.newHadoopConf()), + hadoopConf) + .libhdfs val inputPartitions = op.inputPartitions.map(_.asInstanceOf[FilePartition]) val multiStoreReason = CometScanUtils.multiStoreFallbackReason( "Native Text scan", diff --git a/spark/src/test/scala/org/apache/comet/csv/CometCsvNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/csv/CometCsvNativeReadSuite.scala index eeb809764be..708215459be 100644 --- a/spark/src/test/scala/org/apache/comet/csv/CometCsvNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/csv/CometCsvNativeReadSuite.scala @@ -176,4 +176,25 @@ class CometCsvNativeReadSuite extends CometTestBase { } } } + + test("Native csv read - a libhdfs scheme set only by a read option falls back") { + withTempPath { dir => + spark.range(5).write.csv(dir.toString) + withSQLConf( + CometConf.COMET_CSV_V2_NATIVE_ENABLED.key -> "true", + SQLConf.USE_V1_SOURCE_LIST.key -> "") { + // The option routes `file` paths through libhdfs for this read only. Native cannot read + // them that way, so the scan must stay on Spark, which ignores the option. + val df = spark.read + .schema("id LONG") + .option("fs.comet.libhdfs.schemes", "file,hdfs") + .csv(dir.toString) + val (_, plan) = + checkSparkAnswerAndFallbackReason(df, "listed for libhdfs only through reader options") + assert( + collect(plan) { case s: CometCsvNativeScanExec => s }.isEmpty, + s"Expected Spark's CSV reader but found CometCsvNativeScanExec. Plan:\n$plan") + } + } + } } diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index e39cfc4c5b6..a8dac95bb56 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -22,15 +22,28 @@ package org.apache.comet.text import java.io.File import java.nio.charset.StandardCharsets import java.nio.file.Files +import java.util.concurrent.atomic.AtomicLong +import java.util.zip.GZIPOutputStream -import org.apache.spark.sql.CometTestBase +import org.scalatest.concurrent.Eventually._ +import org.scalatest.time.{Millis, Seconds, Span} + +import org.apache.hadoop.fs.{FileSystem, Path} +import org.apache.hadoop.io.compress.GzipCodec +import org.apache.spark.scheduler.{SparkListener, SparkListenerTaskEnd} +import org.apache.spark.sql.{CometTestBase, DataFrame} import org.apache.spark.sql.comet.CometTextNativeScanExec import org.apache.spark.sql.functions.input_file_name import org.apache.spark.sql.internal.SQLConf -import org.apache.comet.CometConf +import org.apache.comet.{CometConf, ExtendedExplainInfo} import org.apache.comet.CometSparkSessionExtensions.isSpark41Plus +/** A gzip codec whose file extension holds a space, to test codec lookup on decoded paths. */ +class SpacedGzipCodec extends GzipCodec { + override def getDefaultExtension: String = ".packed gz" +} + class CometTextNativeReadSuite extends CometTestBase { private val TEST_TEXT_PATH = "src/test/resources/test-data/text-test-1.txt" @@ -488,4 +501,181 @@ class CometTextNativeReadSuite extends CometTestBase { } } } + + // Plan only: asserts the scan stays on Spark and records why, without running a query that + // Spark itself would fail. + private def assertFallsBack(df: DataFrame, reason: String): Unit = { + withSQLConf(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false") { + val plan = df.queryExecution.executedPlan + assert(plan.collect { case s: CometTextNativeScanExec => s }.isEmpty, plan) + val reasons = new ExtendedExplainInfo().getFallbackReasons(plan) + assert(reasons.exists(_.contains(reason)), reasons) + } + } + + test("native text read - falls back for a local file with a Hadoop checksum file") { + withTempDir { dir => + val hadoopConf = spark.sparkContext.hadoopConfiguration + val path = new Path(new File(dir, "checked.txt").getAbsolutePath) + // Hadoop's local filesystem writes a `.checked.txt.crc` next to the data. + val out = FileSystem.getLocal(hadoopConf).create(path) + out.write("one\n".getBytes(StandardCharsets.UTF_8)) + out.close() + assert(new File(dir, ".checked.txt.crc").exists()) + // Replace the data without updating the checksum: Spark raises a ChecksumException on read. + Files.write(new File(dir, "checked.txt").toPath, "two\n".getBytes(StandardCharsets.UTF_8)) + withTextConf { + assertFallsBack(spark.read.text(path.toString), "Hadoop checksum") + } + } + } + + test("native text read - codec lookup uses the decoded file name") { + withTempDir { dir => + val file = new File(dir, "small.packed gz") + val out = new GZIPOutputStream(Files.newOutputStream(file.toPath)) + out.write("one\ntwo\n".getBytes(StandardCharsets.UTF_8)) + out.close() + val hadoopConf = spark.sparkContext.hadoopConfiguration + val key = "io.compression.codecs" + val previous = hadoopConf.get(key) + hadoopConf.set(key, classOf[SpacedGzipCodec].getName) + try { + withTextConf { + // The URL-encoded name `small.packed%20gz` hides the registered `.packed gz` suffix. + checkSparkAnswerAndFallbackReason( + spark.read.text(file.getAbsolutePath), + "compressed text files") + } + } finally { + if (previous == null) hadoopConf.unset(key) else hadoopConf.set(key, previous) + } + } + } + + test("native text read - a compressed file in a pruned partition does not block native") { + withTempDir { dir => + val hot = new File(dir, "p=hot") + val cold = new File(dir, "p=cold") + hot.mkdirs() + cold.mkdirs() + Files.write(new File(hot, "a.txt").toPath, "alpha\n".getBytes(StandardCharsets.UTF_8)) + val out = new GZIPOutputStream(Files.newOutputStream(new File(cold, "b.txt.gz").toPath)) + out.write("beta\n".getBytes(StandardCharsets.UTF_8)) + out.close() + withTextConf { + val df = spark.read.text(dir.getAbsolutePath).where("p = 'hot'").select("value") + checkSparkAnswerAndOperator(df, Seq(classOf[CometTextNativeScanExec])) + } + } + } + + test("native text read - a listed file whose name object_store rejects falls back") { + withTempDir { dir => + // Legal for Hadoop, rejected by object_store. The directory root alone looks fine. + Files.write(new File(dir, "a\tb.txt").toPath, "expected\n".getBytes(StandardCharsets.UTF_8)) + withTextConf { + checkSparkAnswerAndFallbackReason( + spark.read.text(dir.getAbsolutePath), + "cannot open path") + } + } + } + + test("native text read - falls back for mandatory file-open options in line mode") { + withTempDir { dir => + val file = new File(dir, "lines.txt") + Files.write(file.toPath, "alpha\nbeta\n".getBytes(StandardCharsets.UTF_8)) + withTextConf { + assertFallsBack( + spark.read + .option("mapreduce.job.input.file.must.fs.s3a.readahead.range", "64K") + .text(file.getAbsolutePath), + "mapreduce.job.input.file.must") + } + } + } + + test("native text read - wholetext ignores line.maxlength") { + withTempDir { dir => + val file = new File(dir, "whole.txt") + Files.write(file.toPath, "alpha\nbeta".getBytes(StandardCharsets.UTF_8)) + val key = "mapreduce.input.linerecordreader.line.maxlength" + val hadoopConf = spark.sparkContext.hadoopConfiguration + try { + withTextConf { + // Spark's whole-file reader never reads this setting, so neither a finite limit nor a + // value that is not an integer may fail or fall back a wholetext query. + Seq("5", "64k").foreach { value => + hadoopConf.set(key, value) + checkSparkAnswerAndOperator( + spark.read.option("wholetext", "true").text(file.getAbsolutePath)) + } + } + } finally { + hadoopConf.unset(key) + } + } + } + + test("native text read - scans of the same file compare equal") { + withTempDir { dir => + val file = new File(dir, "lookup.txt") + Files.write(file.toPath, "a\nb\nc\n".getBytes(StandardCharsets.UTF_8)) + withTextConf { + withSQLConf(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false") { + def scan: CometTextNativeScanExec = + spark.read + .text(file.getAbsolutePath) + .queryExecution + .executedPlan + .collect { case s: CometTextNativeScanExec => s } + .head + // Equal canonical forms let Spark reuse the exchanges and stages of a repeated lookup. + assert(scan.sameResult(scan)) + } + } + } + } + + test("native text read - reports input bytes and records to the task metrics") { + withTempDir { dir => + (0 until 4).foreach { i => + Files.write( + new File(dir, s"part-$i.txt").toPath, + "a\nb\nc\n".getBytes(StandardCharsets.UTF_8)) + } + val bytes = new AtomicLong(0) + val records = new AtomicLong(0) + val listener = new SparkListener { + override def onTaskEnd(taskEnd: SparkListenerTaskEnd): Unit = { + if (taskEnd.taskMetrics != null) { + bytes.addAndGet(taskEnd.taskMetrics.inputMetrics.bytesRead) + records.addAndGet(taskEnd.taskMetrics.inputMetrics.recordsRead) + } + } + } + spark.sparkContext.addSparkListener(listener) + try { + withTextConf { + withSQLConf(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false") { + val df = spark.read.text(dir.getAbsolutePath) + // Run only the native query: a Spark pass would add the same bytes and hide a + // missing metric. + assert(df.queryExecution.executedPlan.collect { case s: CometTextNativeScanExec => + s + }.nonEmpty) + assert(df.collect().length == 12) + } + } + // The listener bus is asynchronous, so wait for the four tasks' totals. + eventually(timeout(Span(10, Seconds)), interval(Span(100, Millis))) { + assert(bytes.get() == 24L, s"bytesRead=${bytes.get()}") + assert(records.get() == 12L, s"recordsRead=${records.get()}") + } + } finally { + spark.sparkContext.removeSparkListener(listener) + } + } + } } From cb2dfe052649782868345ddc518d1108f702e691 Mon Sep 17 00:00:00 2001 From: Parth Chandra Date: Mon, 26 Oct 2026 13:39:22 -0700 Subject: [PATCH 12/12] style: drop the redundant Eventually import from the text suite --- .../scala/org/apache/comet/text/CometTextNativeReadSuite.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala index a8dac95bb56..3d2ec890179 100644 --- a/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala +++ b/spark/src/test/scala/org/apache/comet/text/CometTextNativeReadSuite.scala @@ -25,7 +25,6 @@ import java.nio.file.Files import java.util.concurrent.atomic.AtomicLong import java.util.zip.GZIPOutputStream -import org.scalatest.concurrent.Eventually._ import org.scalatest.time.{Millis, Seconds, Span} import org.apache.hadoop.fs.{FileSystem, Path}