zabetak commented on code in PR #5974:
URL: https://github.com/apache/hive/pull/5974#discussion_r2206749502
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java:
##########
@@ -493,4 +494,44 @@ private void checkTablesUsed(String query, Set<String>
tables) throws Exception
Assert.assertEquals(new TreeSet<>(tables), new TreeSet<>(result));
}
+
+ @Test
+ public void testValidateJDBCTablePropertiesAreNonAmbiguous() {
Review Comment:
The method seems to contain three tests and not just one so it would be
better to split and name them accordingly so that when something fails we know
exactly where the problem is.
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java:
##########
@@ -493,4 +494,44 @@ private void checkTablesUsed(String query, Set<String>
tables) throws Exception
Assert.assertEquals(new TreeSet<>(tables), new TreeSet<>(result));
}
+
+ @Test
+ public void testValidateJDBCTablePropertiesAreNonAmbiguous() {
Review Comment:
Additionally, I am not sure if this is the best class to put these new tests
but let's revisit this once we agree on the prod changes.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -13875,6 +13875,8 @@ private Map<String, String>
validateAndAddDefaultProperties(
boolean isTemporaryTable, boolean isTransactional, boolean isManaged,
String[] qualifiedTabName, boolean isTableTypeChanged) throws SemanticException
{
Map<String, String> retValue =
Optional.ofNullable(tblProp).orElseGet(HashMap::new);
+ validateJDBCTablePropertiesAreNonAmbiguous(isExt, storageFormat, retValue);
Review Comment:
This verification check seems to be specific to the JDBC storage handler so
it would fit better if the checks were delegated there (or to some other JDBC
related class). If we start adding storage handler specific behavior here the
class will start getting tightly coupled with storage that we probably don't
want.
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java:
##########
@@ -493,4 +494,44 @@ private void checkTablesUsed(String query, Set<String>
tables) throws Exception
Assert.assertEquals(new TreeSet<>(tables), new TreeSet<>(result));
}
+
+ @Test
+ public void testValidateJDBCTablePropertiesAreNonAmbiguous() {
+ StorageFormat storageFormat = mock(StorageFormat.class);
Review Comment:
Is it complicated to create a new instance of
`StorageFormat/JdbcStorageHandler`? There are various drawback when we use
mocks (debugging complexity, library dependencies, reduce test coverage, etc.)
so we should strive to avoid it unless it is really complicated to create
"real" objects.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
