Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to set custom metastore in HiveQueryRunner #3009

Conversation

kokosing
Copy link
Member

@kokosing kokosing commented Mar 5, 2020

Allow to set custom metastore in HiveQueryRunner

@cla-bot cla-bot bot added the cla-signed label Mar 5, 2020
@kokosing kokosing requested a review from sopel39 March 5, 2020 14:03
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure tests pass

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just skimming)

metastore.createDatabase(identity, createDatabaseMetastoreObject(TPCH_SCHEMA));
copyTpchTables(queryRunner, "tpch", TINY_SCHEMA_NAME, createSession(Optional.empty()), tables);
}
public Builder setTables(Iterable<TpchTable<?>> tables)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setInitialTables

.setExtraProperties(extraProperties)
.setBaseDataDir(baseDataDir)
.build();
public Builder setExtraHiveProperties(Map<String, String> extraHiveProperties)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setHiveProperties? not sure why we had "extra" here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there are some defaults. And it was named like this before.

public static DistributedQueryRunner createQueryRunner(Iterable<TpchTable<?>> tables, Map<String, String> extraProperties, Optional<Path> baseDataDir)
throws Exception
public static class Builder
extends DistributedQueryRunner.Builder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you inherit parent builder, you should pass your own Builder type there and the parent builder should return T from build methods... otherwise code using builder needs to use methods in certain specific order, not the way builder should work

follow-up maybe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this. It looks like a follow-up

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map<String, String> hiveProperties = ImmutableMap.<String, String>builder()
.put("hive.time-zone", TIME_ZONE.getID())
.put("hive.max-partitions-per-scan", "1000")
.put("hive.assume-canonical-partition-keys", "true")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this default?? TODO maybe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to be default.

@kokosing kokosing force-pushed the origin/master/233_hive_query_runner_metastore branch from b9ce77c to 05cf6ba Compare March 5, 2020 19:29
@kokosing kokosing merged commit 6509248 into trinodb:master Mar 5, 2020
@kokosing kokosing deleted the origin/master/233_hive_query_runner_metastore branch March 5, 2020 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants