HIVE-29032: Enhance qt:database option to expose connection properties in qfiles #5919
Conversation
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
|
| * Syntax: qt:database:DatabaseType:path_to_init_script | ||
| * | ||
| * The database type ({@link DatabaseType}) is obligatory argument. The initialization script can be omitted. | ||
| * Syntax: qt:database:DatabaseType:databaseName:path_to_init_script |
There was a problem hiding this comment.
Can we make it a bit more user-friendly?
qt:database:DatabaseType:schema:DatabaseName:path_to_init_script
--!qt:database:mysql:schema:qdb:q_test_author_book_tables.sql
There was a problem hiding this comment.
This will make the option parsing trickier and maybe it's not worth it given that most devs will copy-paste from one file to another.
For the record, I also considered using commons-cli and Options to allow the following syntax:
qt:database:--type mysql --name qdb --script q_test_author_book_tables.sql
qt:database:-t mysql -n qdb -s q_test_author_book_tables.sql
But it felt a bit of an overkill especially since databaseType and databaseName are mandatory args.
If the rest of the changes in this PR look good how about merging this (since tests are green) and following up with another ticket if you feel that its worth it?
There was a problem hiding this comment.
ok, though I like this syntax more:
qt:database:--type mysql --name qdb --script q_test_author_book_tables.sql
| @@ -82,52 +80,48 @@ AbstractExternalDB create() { | |||
| abstract AbstractExternalDB create(); | |||
| } | |||
|
|
|||
| private final Map<DatabaseType, String> databaseToScript = new EnumMap<>(DatabaseType.class); | |||
| private final String scriptsDir; | |||
| private final List<AbstractExternalDB> databases = new ArrayList<>(); | |||
There was a problem hiding this comment.
@zabetak, should it be a Set? What if the used defines init scripts for multiple schemas?
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
No, it only affects the test infrastructure.
How was this patch tested?