Hive: Support case insensitive in hive query #2053

dixingxing0 · 2021-01-08T11:35:36Z

Fixes #2045.
Since hive is case insensitive, we will make HiveIcebergSerDe always case insensitive by set iceberg.mr.case.sensitive to false.

@pvary, could you review this please?

pvary · 2021-01-08T11:46:40Z

mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergSerDe.java

-    Schema projectedSchema = selectedColumns.length > 0 ? tableSchema.select(selectedColumns) : tableSchema;
+    Schema projectedSchema = tableSchema;
+
+    boolean caseSensitive = configuration.getBoolean(InputFormatConfig.CASE_SENSITIVE,


I understand that we want to have a possibility to configure the IcebergInputFormat to be case sensitive or case insensitive since it can be used by other MR jobs as well. Do we want to allow the users of Hive to shot themselves on the foot and enable case sensitivity?
My first guess would be that we should not use the configuration here, just go with false, but if you have some specific use-case in your mind I can be easily convinced 😄

Agree with you, i'll make here go with false, i don't have a specific use-case.
Since we make hive go with false, i think iceberg.mr.case.sensitive should use true as a default value, is that ok?

pvary · 2021-01-08T11:47:24Z

...main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergRecordObjectInspector.java


  private final List<IcebergRecordStructField> structFields;
+  private final List<IcebergRecordStructField> structFieldsInLowercase;


Why do we keep both of the fields? Shouldn't we just keep the currently requested version?

You are right! I'll change it.

…as default value for iceberg.mr.case.sensitive

dixingxing0 · 2021-01-08T12:58:42Z

@pvary, i just pushed a commit, could you please take a look?

pvary · 2021-01-08T12:58:48Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithEngine.java

@@ -150,6 +150,27 @@ public void testScanTable() throws IOException {
    Assert.assertArrayEquals(new Object[] {"Alice", 0L}, descRows.get(2));
  }

+  @Test


I think these queries do not use the execution engine itself, so we do not have to put these tests to this class.
It is ok to have them only in the TestHiveIcebergStorageHandlerLocalScan.java

OK, i'll remove the test code in TestHiveIcebergStorageHandlerLocalScan.java

pvary · 2021-01-08T13:01:30Z

@pvary, i just pushed a commit, could you please take a look?

Just answered 😄
Since the queries in the new test cases do not use the execution engine itself (they are using only local tasks), we do need them in both classes. It is enough to have them in the TestHiveIcebergStorageHandlerLocalScan.java

dixingxing0 · 2021-01-08T13:03:21Z

@pvary, i just pushed a commit, could you please take a look?

Just answered 😄
Since the queries in the new test cases do not use the execution engine itself (they are using only local tasks), we do need them in both classes. It is enough to have them in the TestHiveIcebergStorageHandlerLocalScan.java

Thanks, i've removed the test code in TestHiveIcebergStorageHandlerWithEngine.

pvary · 2021-01-08T13:24:02Z

mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergSerDe.java

@@ -80,12 +80,14 @@ public void initialize(@Nullable Configuration configuration, Properties serDePr
        tableSchema = hiveSchemaOrThrow(serDeProperties, e);
      }
    }
+    configuration.set(InputFormatConfig.CASE_SENSITIVE, "false");


configuration.setBoolean(InputFormatConfig.CASE_SENSITIVE, false);

pvary · 2021-01-08T13:25:17Z

...main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergRecordObjectInspector.java

+        Types.NestedField fieldInLowercase = Types.NestedField.of(field.fieldId(), field.isOptional(),
+                field.name().toLowerCase(), field.type(), field.doc());
+        structField = new IcebergRecordStructField(fieldInLowercase, oi, position);
+      }


nit: After if blocks we tend to leave an extra line

pvary · 2021-01-08T13:26:00Z

mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergInputFormat.java

+      this.caseSensitive = conf.getBoolean(InputFormatConfig.CASE_SENSITIVE,
+              InputFormatConfig.CASE_SENSITIVE_DEFAULT);


nit: This can be a single line

pvary · 2021-01-08T13:26:18Z

mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergInputFormat.java

+      if (selectedColumns == null) {
+        return tableSchema;
+      }
+      return caseSensitive ? tableSchema.select(selectedColumns) : tableSchema.caseInsensitiveSelect(selectedColumns);


nit: Extra line

pvary · 2021-01-08T13:28:02Z

@pvary, i just pushed a commit, could you please take a look?

Just answered 😄
Since the queries in the new test cases do not use the execution engine itself (they are using only local tasks), we do need them in both classes. It is enough to have them in the TestHiveIcebergStorageHandlerLocalScan.java

Thanks, i've removed the test code in TestHiveIcebergStorageHandlerWithEngine.

Thanks @dixingxing0 for the patch!

Few last nits and then I think it is good.
+1 non-binding

@rdblue: Could you please review?

Thanks,
Peter

dixingxing0 · 2021-01-08T14:58:42Z

@pvary, thank you for your patience, i will be more careful next time.

rdblue · 2021-01-11T19:10:51Z

mr/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergObjectInspector.java

+    return create(schema, true);
+  }
+
+  public static ObjectInspector create(@Nullable Schema schema, boolean caseSensitive) {


Why accept a case sensitivity setting for something that is only used by Hive? I thought that the purpose of this PR was to allow case sensitive behavior for the InputFormat, but not Hive because Hive's behavior is case insensitive.

Thanks, comment addressed.

rdblue · 2021-01-15T17:31:06Z

@dixingxing0, this looks good to me now. Could you resolve the conflicts? I can merge after that. Thanks!

dixingxing0 · 2021-01-16T02:07:31Z

@dixingxing0, this looks good to me now. Could you resolve the conflicts? I can merge after that. Thanks!

Conflincts resolved.

rdblue · 2021-01-16T21:30:57Z

Thanks, @dixingxing0!

dixingxing0 · 2021-01-19T10:35:54Z

@pvary @rdblue Thanks for the review and all the help 😄 .

Co-authored-by: dixingxing <dixingxing@autohome.com.cn>

Support case insensitive in hive query.

6a0d726

github-actions bot added the MR label Jan 8, 2021

dixingxing0 mentioned this pull request Jan 8, 2021

Hive: query failed when iceberg table has upper case column #2045

Closed

pvary reviewed Jan 8, 2021

View reviewed changes

Set iceberg.mr.case.sensitive to false in HiveIcebergSerDe; Use true …

e3f56dd

…as default value for iceberg.mr.case.sensitive

pvary reviewed Jan 8, 2021

View reviewed changes

Remove the test code in TestHiveIcebergStorageHandlerWithEngine

ed3e4e5

pvary reviewed Jan 8, 2021

View reviewed changes

Minor changes.

3ff0939

rdblue reviewed Jan 11, 2021

View reviewed changes

dixingxing0 added 2 commits January 12, 2021 09:43

Make case insensitive for Hive.

26f311c

Improve Code style.

4c97c6e

pvary mentioned this pull request Jan 12, 2021

Hive: Fix Deserializer to use source deserializer instead of the Iceberg ones #2078

Merged

pvary approved these changes Jan 15, 2021

View reviewed changes

Merge branch 'master' into hive-case-insensitive

81e5d19

rdblue merged commit b1de539 into apache:master Jan 16, 2021

XuQianJin-Stars pushed a commit to XuQianJin-Stars/iceberg that referenced this pull request Mar 22, 2021

Hive: Queries should be case insensitive (apache#2053)

0570407

Co-authored-by: dixingxing <dixingxing@autohome.com.cn>

uncleGen mentioned this pull request Jun 21, 2021

fix inconsistent case type of field name #2717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hive: Support case insensitive in hive query #2053

Hive: Support case insensitive in hive query #2053

dixingxing0 commented Jan 8, 2021 •

edited

Loading

pvary Jan 8, 2021

dixingxing0 Jan 8, 2021 •

edited

Loading

pvary Jan 8, 2021

dixingxing0 Jan 8, 2021 •

edited

Loading

dixingxing0 commented Jan 8, 2021

pvary Jan 8, 2021

dixingxing0 Jan 8, 2021

pvary commented Jan 8, 2021

dixingxing0 commented Jan 8, 2021

pvary Jan 8, 2021 •

edited

Loading

pvary Jan 8, 2021

dixingxing0 Jan 8, 2021

pvary Jan 8, 2021

pvary Jan 8, 2021

pvary commented Jan 8, 2021

dixingxing0 commented Jan 8, 2021

rdblue Jan 11, 2021

dixingxing0 Jan 12, 2021

rdblue commented Jan 15, 2021

dixingxing0 commented Jan 16, 2021 •

edited

Loading

rdblue commented Jan 16, 2021

dixingxing0 commented Jan 19, 2021


		private final List<IcebergRecordStructField> structFields;
		private final List<IcebergRecordStructField> structFieldsInLowercase;

		this.caseSensitive = conf.getBoolean(InputFormatConfig.CASE_SENSITIVE,
		InputFormatConfig.CASE_SENSITIVE_DEFAULT);

Hive: Support case insensitive in hive query #2053

Hive: Support case insensitive in hive query #2053

Conversation

dixingxing0 commented Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

dixingxing0 Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dixingxing0 Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

dixingxing0 commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary commented Jan 8, 2021

dixingxing0 commented Jan 8, 2021

pvary Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary commented Jan 8, 2021

dixingxing0 commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Jan 15, 2021

dixingxing0 commented Jan 16, 2021 • edited Loading

rdblue commented Jan 16, 2021

dixingxing0 commented Jan 19, 2021

dixingxing0 commented Jan 8, 2021 •

edited

Loading

dixingxing0 Jan 8, 2021 •

edited

Loading

dixingxing0 Jan 8, 2021 •

edited

Loading

pvary Jan 8, 2021 •

edited

Loading

dixingxing0 commented Jan 16, 2021 •

edited

Loading