Remove HiveConfig from Iceberg module by homar · Pull Request #12506 · trinodb/trino

homar · 2022-05-22T09:25:47Z

Description

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

findepi · 2022-05-23T13:20:58Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadataFactory.java

It's not metastore-related. isTranslateHiveViews is used in HiveMetadata.

What would happen if you move this back to HiveConfig?

findepi · 2022-05-23T13:28:21Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

I guess this use-place is the reason why you moved the config from HiveConfig to metastoreConfig.

However, we don't need this configurable in iceberg. There is no concept of "hive views" in Iceberg.

we may be able to solve this using a technique like @MaxDomainCompactionThreshold

add a TestIcebergPlugin test that configuring connector with hive.hive-views.enabled fails

Ok my bad, I was pretty sure iceberg needs this. Thanks

findinpath · 2022-05-25T10:23:14Z

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.9.0:compile (default-compile) on project trino-hive: Compilation failure
[ERROR] /Users/marius/Development/starburst/trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadataFactory.java:[110,27] cannot find symbol
[ERROR]   symbol:   method isTranslateHiveViews()
[ERROR]   location: variable hiveConfig of type io.trino.plugin.hive.HiveConfig

build is failing locally.

Do note that the branch has conflicts with master and probably this is why the maven checks tasks were not triggered.

findepi · 2022-05-25T12:41:09Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeModule.java

Looks like extension point, but Delta doesn't need to create extension points. At best, Hive would do this.

what about

binder.bind(Key.get(boolean.class, TranslateHiveViews.class)).toInstance(false);

oh ok, I see now. thanks

tough I guess it is not needed for delta lake as there is still hive config there

findepi · 2022-05-25T12:41:15Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java

Why removed?

because I didn't understand the idea...

findepi · 2022-05-25T12:41:53Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadataFactory.java

Why Optional? it's always either true or false

Suggested change

@TranslateHiveViews Optional<Boolean> translateHiveViews)

@TranslateHiveViews boolean translateHiveViews)

also, this class takes HiveConfig hiveConfig, so if the fields goes back there (as it should), remove the new paramater.

yep I 've got it now

findepi · 2022-05-25T12:43:15Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

Why Optional? it's always either true or false

Suggested change

@TranslateHiveViews Optional<Boolean> translateHiveViews,

@TranslateHiveViews boolean translateHiveViews)

findepi · 2022-05-25T12:44:45Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergPlugin.java

Define a constant so that the test always uses a correct config property name

alexjo2144 · 2022-05-26T15:22:06Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java

If this is Thrift specific, it can go in IcebergHiveMetastoreCatalogModule

i see thjis added to IcebergHiveMetastoreCatalogModule.java but not removed here

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeModule.java

findepi · 2022-05-27T09:37:40Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

use current code default

new HiveConfig().isTranslateHiveViews()

see #12343 (comment)

findepi · 2022-05-27T09:37:45Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHiveFileSystem.java

new HiveConfig().isTranslateHiveViews()

findepi · 2022-05-27T09:37:53Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java

new HiveConfig().isTranslateHiveViews()

findepi · 2022-05-27T09:38:02Z

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java

new HiveConfig().isTranslateHiveViews()

findepi · 2022-05-27T09:38:08Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/S3HiveQueryRunner.java

new HiveConfig().isTranslateHiveViews()

findepi · 2022-05-27T09:38:55Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java

i see thjis added to IcebergHiveMetastoreCatalogModule.java but not removed here

findepi · 2022-05-27T09:39:51Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergPlugin.java

The constant has value only if used in @Config annotation value of the io.trino.plugin.hive.HiveConfig#setTranslateHiveViews method.

findepi · 2022-05-27T15:10:43Z

CI #11848

colebow · 2022-05-31T15:39:24Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

+    }
+
+    @LegacyConfig("hive.target-max-file-size")
+    @Config("iceberg.target-max-file-size")


If we're changing config properties, we should also document this change. @homar can you submit a follow-up PR with docs changes, or would you like me to take the lead on that?

cc @findepi

I will submit a pr. Thanks for pointing it out

@colebow I made a PR #12619

cla-bot bot added the cla-signed label May 22, 2022

homar force-pushed the homar/remove_hiveconfig_from_iceberg_module branch from ba87988 to fbcc078 Compare May 22, 2022 10:55

github-actions bot added the tests:hive label May 22, 2022

homar force-pushed the homar/remove_hiveconfig_from_iceberg_module branch from fbcc078 to 4e2b9c2 Compare May 22, 2022 12:48

homar changed the title ~~[WIP} Remove HiveConfig from Iceberg module~~ [WIP] Remove HiveConfig from Iceberg module May 23, 2022

homar marked this pull request as ready for review May 23, 2022 11:52

homar changed the title ~~[WIP] Remove HiveConfig from Iceberg module~~ Remove HiveConfig from Iceberg module May 23, 2022

homar requested review from alexjo2144, ebyhr, findepi and findinpath and removed request for findepi May 23, 2022 11:53

findepi reviewed May 23, 2022

View reviewed changes

homar force-pushed the homar/remove_hiveconfig_from_iceberg_module branch from 4e2b9c2 to a004720 Compare May 25, 2022 08:24

homar force-pushed the homar/remove_hiveconfig_from_iceberg_module branch 2 times, most recently from df9d6b6 to 5f1dab8 Compare May 25, 2022 12:28

findepi requested changes May 25, 2022

View reviewed changes

homar force-pushed the homar/remove_hiveconfig_from_iceberg_module branch 2 times, most recently from d42c963 to 358abb3 Compare May 26, 2022 14:27

alexjo2144 reviewed May 26, 2022

View reviewed changes

homar force-pushed the homar/remove_hiveconfig_from_iceberg_module branch from 358abb3 to f58e8b1 Compare May 26, 2022 15:56

findepi reviewed May 27, 2022

View reviewed changes

Remove HiveConfig from Iceberg module

b11f77f

homar force-pushed the homar/remove_hiveconfig_from_iceberg_module branch from f58e8b1 to b11f77f Compare May 27, 2022 13:32

findepi approved these changes May 27, 2022

View reviewed changes

empty

bf7f4ef

findepi added the no-release-notes This pull request does not require release notes entry label May 28, 2022

findepi merged commit cf29082 into trinodb:master May 28, 2022

github-actions bot added this to the 383 milestone May 28, 2022

colebow mentioned this pull request May 31, 2022

Add Trino 383 release notes #12565

Merged

colebow reviewed May 31, 2022

View reviewed changes

ebyhr mentioned this pull request Jun 1, 2022

Add docs for iceberg parameters extracted from hive config #12619

Merged

homar mentioned this pull request Jun 1, 2022

Release notes for 383 #12556

Closed

findepi mentioned this pull request Jun 10, 2022

Reject Hive configuration properties that have no meaning for Iceberg #9607

Closed

	@TranslateHiveViews Optional<Boolean> translateHiveViews)
	@TranslateHiveViews boolean translateHiveViews)

Conversation

homar commented May 22, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findinpath commented May 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented May 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants