andygrove opened a new issue, #1423:
URL: https://github.com/apache/arrow-java/issues/1423
### Describe the enhancement requested
`Data.importField` (and therefore `Data.importVector` /
`Data.importIntoVector`) creates a new `SchemaImporter` on every call:
```java
public static Field importField(BufferAllocator allocator, ArrowSchema
schema,
CDataDictionaryProvider provider, boolean closeImportedStructs) {
try {
SchemaImporter importer = new SchemaImporter(allocator);
return importer.importField(schema, provider);
} ...
```
`SchemaImporter` owns the dictionary id counter (`private long
nextDictionaryID = 0L`), so every top-level `importField` call starts numbering
dictionaries from 0. When a caller imports several columns one at a time (one
`ArrowArray`/`ArrowSchema` pair per column) into the same
`CDataDictionaryProvider`, the second dictionary-encoded column gets
`DictionaryEncoding` id 0 again. `CDataDictionaryProvider.put` then replaces
the first column's dictionary and closes its vector:
```java
void put(Dictionary dictionary) {
Dictionary previous = map.put(dictionary.getEncoding().getId(),
dictionary);
if (previous != null) {
previous.getVector().close();
}
}
```
The first column's indices now resolve against the wrong dictionary (or a
closed one), which corrupts data silently.
Importing the columns as a single struct via `Data.importSchema` /
`Data.importVectorSchemaRoot` avoids this because one `SchemaImporter` handles
all children, but that is not always an option. In Apache DataFusion Comet, the
native side exports each column separately, so Comet had to work around this by
shipping its own `ArrowImporter` class in the `org.apache.arrow.c` package
(https://github.com/apache/datafusion-comet/blob/main/spark/src/main/java/org/apache/arrow/c/ArrowImporter.java).
It holds one `SchemaImporter` for the lifetime of a batch so dictionary ids
stay unique across columns. It has to live in `org.apache.arrow.c` because
`SchemaImporter` and `ArrayImporter` are package-private.
### Proposal
Expose a public API that lets a caller import multiple fields/arrays with
shared dictionary id state, for example one of:
1. Make `SchemaImporter` (or a new wrapper around it) public so a caller can
construct one and call `importField` repeatedly, with the dictionary counter
carried across calls.
2. Add overloads of `Data.importField` / `Data.importVector` that accept a
reusable importer or an explicit starting dictionary id.
3. Have `Data.importField` derive the dictionary id from the ids already
present in the supplied `CDataDictionaryProvider` (e.g. `max(existing ids) +
1`) instead of a fresh counter, so repeated calls against the same provider
never collide.
Option 3 is the least invasive and fixes existing callers without an API
change, though it changes the ids that get assigned. Option 1 or 2 would let
Comet delete its copy of the importer and stop depending on package-private
classes.
Happy to put up a PR if there is agreement on the approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
