chore: various refactoring changes for iceberg [iceberg] #2680
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2680 +/- ##
============================================
+ Coverage 56.12% 57.14% +1.01%
- Complexity 976 1386 +410
============================================
Files 119 149 +30
Lines 11743 13930 +2187
Branches 2251 2391 +140
============================================
+ Hits 6591 7960 +1369
- Misses 4012 4751 +739
- Partials 1140 1219 +79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
native/core/Cargo.toml
Outdated
| hdfs-sys = {version = "0.3", optional = true, features = ["hdfs_3_3"]} | ||
| opendal = { version ="0.54.1", optional = true, features = ["services-hdfs"] } | ||
| uuid = "1.0" | ||
| opendal = { version ="0.54.0", optional = true, features = ["services-hdfs"] } |
There was a problem hiding this comment.
is there a reason for this change? Comet could still choose to use 0.54.1 since it is semver compatible
There was a problem hiding this comment.
Looks like this happened due to rebasing. Reverted.
| this.dataSchema = dataSchema; | ||
| this.isCaseSensitive = isCaseSensitive; | ||
| this.useFieldId = useFieldId; | ||
| this.ignoreMissingIds = ignoreMissingIds; |
There was a problem hiding this comment.
| this.ignoreMissingIds = ignoreMissingIds; | |
| this.ignoreMissingIds = ignoreMissingIds; | |
| this.useLegacyDateTimestamp = useLegacyDateTimestamp; |
| this.ignoreMissingIds = ignoreMissingIds; | ||
| this.partitionSchema = partitionSchema; | ||
| this.partitionValues = partitionValues; | ||
| this.preInitializedReaders = preInitializedReaders; |
There was a problem hiding this comment.
| this.preInitializedReaders = preInitializedReaders; | |
| this.preInitializedReaders = preInitializedReaders; | |
| this.metrics.clear(); | |
| if (metrics != null) { | |
| this.metrics.putAll(metrics); | |
| } |
| filteredSchema = filteredSchema.add(sparkFields[i]); | ||
| } | ||
| } | ||
| sparkSchema = filteredSchema; |
There was a problem hiding this comment.
Is it possible that the filtering done here may lead to ArrayIndexOutOfBoundsException at https://github.com/parthchandra/datafusion-comet/blob/d73bcbab9f80836d7229207f309283942501e9ab/common/src/main/java/org/apache/comet/parquet/NativeBatchReader.java#L985 ?
Now the sparkSchema may have less fields than before I see no new logic to protect the .fields()[i] call there.
There was a problem hiding this comment.
Yes, you're right. This is not entirely correct. Let me fix this.
There was a problem hiding this comment.
Yup. Fixed to match the fields by name.
| import org.apache.spark.sql.types.StructType; | ||
|
|
||
| /** | ||
| * A specialized NativeBatchReader for Iceberg that accepts ParquetMetadata as a JSON string. This |
There was a problem hiding this comment.
accepts ParquetMetadata as a JSON string - actually it accepts byte[] parquetMetadataBytes at https://github.com/apache/datafusion-comet/pull/2680/files#diff-e57878f6cd8036999500de5719f8f4bbe28e1ed5dcb79a02ad7d7eb206f37473R44, i.e. not a String but bytes.
There was a problem hiding this comment.
Thank you for catching this. The first version I did used JSON, but this is more efficient.
andygrove
changed the title
|
@parthchandra You said |
Oops. I had pushed to the wrong branch :(. Corrected. |
4 participants
Which issue does this PR close?
Part of the changes needed for #2060
Mostly does cleanup of the
native_iceberg_compatAPIs so the they do not have Parquet classes. As a plus provides a utility class to allow ParquetMetadata to be serialized and deserialized to/from the Thrift format. This will also be useful in passing ParquetMetadata from JVM to native (for all native scan implementations). Currently the native scans end up reading Parquet metadata again (even though it has already been read in the JVM side) and this can be a costly operation in object stores.