Support changing column types in Hive connector

[ebyhr]

Description

Support changing column types in Hive connector

Release notes

(x) Release notes are required, with the following suggested text:

# Hive
* Support changing column types. ({issue}`15515`)

[raunaqmorarka]

I suspect this approach is going to be slower because of the additional null check. You can try adding a JMH benchmark like BenchmarkOrcDecimalReader and compare the performance of these approaches.
We're also not preserving the mayHaveNull of the original block, that should definitely be done.
We did these type of adaptations in column readers rather than the page source in the parquet reader as well.

[Praveen2112]

But having all the adaptations in column reader won't it be a bit messy or can we have an adaptations which could be reused by parquet and orc file format.

And some cases of coercion is supported by hive but it could be restricted by iceberg or delta (not sure) - how could we handle them in these case ?

[raunaqmorarka]

The primitive types of orc and parquet are different so the required adaptations won't be the same (e.g. there is no byte/short in parquet only int32/int64).
We could create a function in ByteColumnReader to avoid repeating code like below

if (type == TINYINT) {
      return new ByteArrayBlock(nextBatchSize, Optional.empty(), values);
}
 if (type == INTEGER) {
      return new IntArrayBlock(nextBatchSize, Optional.empty(), convertToIntArray(values));
}
throw new VerifyError("Unsupported type " + type);

These types of column adaptations which are done in the reader are simple ones meant to resolve minor differences in file format type and trino type (e.g. differing byte count of numbers, difference in precision of decimals, timestamps etc.). These should make sense for any connector. For anything complex like reading a number in the file format as a varchar trino type, which a connector may or may not want to do, can be done in the connector.

[findepi]

Don't require iceberg.id attribute in ORC reader

If I am understanding the change correctly, it could be titled "Fix reading ORC files after column evolved from tinyint to integer"

Am i reading this right?

[ebyhr]

Sorry for my misleading commit tile. Updated it.

[findepi]

Thanks for changing the commit title.

Is TestHiveTransactionalTable.java the only test coverage we have for this "Fix reading ORC files after column evolved from tinyint to integer" change?

[findepi]

Convert ORC block within ColumnAdaptation

The commit message bears no rational. in fact, what's the benefit of the change?

[findepi]

i though the purpose of "Convert ORC block within ColumnAdaptation" was to have readers simplified, so surprised to see this change here.

I think I like the non-composed (without column adaptations) approach better though, so no changes requested here.

[findepi]

if this had to be of top performance, this probably would want to have a simpler loop when ! block.mayHaveNull.

no change requested, since I don't know the perf expectations

[findepi]

BTW would it work to avoid the if here?

isNull[i] = block.isNull(i)
values[i] = block.getByte(i, 0);

cc @dain @sopel39 @raunaqmorarka may know if it is legit

[raunaqmorarka]

I don't get what was gained by adding abstractions at page source layer for these column adaptations rather than letting the column reader handle it as it does today.
My expectation is that there should be no perf regression from the existing approach (demonstrated through JMH results).
Preserving the mayHaveNull of the original block is definitely required, downstream operators take advantage of that to improve performance.
Avoiding branch in the above way looks okay to me, however the contract in Block doesn't explcitly specify any expected behaviour for getter on null positions. So I don't know if that can be safely relied on or in future it may change to throw an exception.

[findepi]

This probably belongs to "Support type evolution from tinyint to smallint and bigint in ORC" (currently in "Add test cases for changing numeric column types")

[ebyhr]

Rebased on master to resolve conflicts and added test cases for storage formats.

[findepi]

is the check effective?
can a partitioned table have partitions with different file format?

[findepi]

btw it's looks like we should be able to lift the limitation for eg TEXTFILE easily.
please add a code comment explaining the logic, why ORC/PARQUET only and what is our stance about partitioned tables.

[findepi]

"Cannot prepare test data with REGEX table"

[findepi]

This is quite a few test cases.
For AVRO, RCBINARY, RCTEXT, SEQUENCEFILE, JSON, TEXTFILE, CSV, [REGEX] should be enough to have one test case eg changing from integer to bigint and checking message is "Unsupported storage format for changing column type"
(then you can simplify pattern in verifySetColumnTypeFailurePermissible)

[findepi]

Optional<String> tableProperty -> String tableProperties
(empty string means "no properties", no need to wrap in optional)

[findepi]

withTableProperty(foo) should be equivalent do instnatiating new SetColumnTypeSetup(.... , foo)
currently the former wraps with WITH (..)

move the SQL formatting logic to where it is used (will add separate comment there)

[findepi]

tableConfiguration = "";
if (!setup.tableProperties.isEmpty()) {
  tableConfiguration += " WITH(%s)".formatted(setup.tableProperties);
}
... new TestTable(..., tableConfiguration + " AS SELECT

[findepi]

pre-existing, but since you're changing this line.... see #15637 (comment)

[ebyhr]

Sent #16376

[findepi]

Thanks for changing the commit title.

Is TestHiveTransactionalTable.java the only test coverage we have for this "Fix reading ORC files after column evolved from tinyint to integer" change?

[findepi]

we should fail for CSV files too (csv is all varchar)

[findepi]

Partition names = names of partitions != names of partitioning columns

[findepi]

Is truncation allowed?
Add a comment

[findepi]

we expect at most one. i guess we should fail when duplicates

.collect(toOptional());

[findepi]

Does this check buy us anything wrt the check already done in HiveMetadata?

[findepi]

Does this check buy us anything wrt the check already done in HiveMetadata?

[findepi]

onHive returns a String output from a CLI
this isn't really meant to run test queries

let's use product tests setup for that, where we have Hive JDBC and we can run queries normally, without processing textual output of some foreign CLI tool

[findepi]

Is there some existing test class which we could add the test to?
would be nice to avoid a minute-long setup just to run a second-long test case

[ebyhr]

I can't find the existing test class except for TestHiveGlueMetastore extending AbstractTestHiveLocal. Do you want me to use the class instead? I avoided the class because I prefer query-based tests.

[findepi]

I am surprised

Firstl, i wouldn't expect existing partitions to be updated at all.

Second, it will make it impossible to update type in large tables.

Let's talk more about this

[ebyhr]

Closing because of less demand.

[wookiist]

@ebyhr Hello!
Is the code that has been committed so far sufficient to make the feature work? Or does it require further implementation?
I was following this pr because my team has needs to use this feature, but it was closed 😭