[HUDI-8533] Bloom functional Index creation without function fails #12290

lokeshj1703 · 2024-11-19T06:52:43Z

Change Logs

Allow creation of functional index without an input function. The PR uses a default function in such a case.

Impact

NA

Risk level (write none, low medium or high below)

low

Documentation Update

NA

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

vinothchandar

Basic question here.. Should this case - bloom_filters without a function defined be handled by BloomFiltersIndexSupport instead of adding an identity function as default.. and making everything treated as functional index?

(Code maintainability wise, I like treating everything as having a function, using identity as needed, including secondary_index. but the functional index code path has a bunch of restrictions?)

vinothchandar · 2024-11-19T13:28:15Z

...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java

      dataMetaClient.getTableConfig().setMetadataPartitionState(dataMetaClient, partitionPath, false);
+      if (MetadataPartitionType.isGenericIndex(partitionPath)) {


is this unit tested?

vinothchandar · 2024-11-19T13:29:43Z

hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java

@@ -90,6 +90,7 @@
 import static org.apache.hudi.common.util.StringUtils.getUTF8Bytes;
 import static org.apache.hudi.common.util.ValidationUtils.checkArgument;
 import static org.apache.hudi.common.util.ValidationUtils.checkState;
+import static org.apache.hudi.index.functional.HoodieFunctionalIndex.SPARK_IDENTITY;


why is this called SPARK_IDENTITY .. and in hudi-common.. this module must be strictly engine free

vinothchandar

This needs some work. I suggest we first focus on addressing #12266 and land that, to make this smaller.. and fix remaining issues.

vinothchandar · 2024-11-20T12:20:32Z

...t/hudi-spark-client/src/main/java/org/apache/hudi/client/utils/SparkMetadataWriterUtils.java

@@ -150,21 +151,22 @@ public static HoodieData<HoodieRecord> getFunctionalIndexRecordsUsingColumnStats
  }

  public static HoodieData<HoodieRecord> getFunctionalIndexRecordsUsingBloomFilter(Dataset<Row> dataset, String columnToIndex,
-                                                                                   HoodieWriteConfig metadataWriteConfig, String instantTime) {
+                                                                                   HoodieWriteConfig metadataWriteConfig, String instantTime, String indexName) {


can we UT this method

vinothchandar · 2024-11-20T12:21:43Z

hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java

+   *
+   * @param indexName Name of the index
+   */
+  public void deleteIndexDefinition(String indexName) {


same. UT please

vinothchandar · 2024-11-20T12:23:00Z

hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java

-    if (!dataMetaClient.getTableConfig().isMetadataPartitionAvailable(MetadataPartitionType.BLOOM_FILTERS)) {
-      LOG.error("Metadata bloom filter index is disabled!");
+    if (!dataMetaClient.getTableConfig().getMetadataPartitions().contains(metadataPartitionName)) {
+      LOG.error("Metadata partition not found {}", metadataPartitionName);


why make the error message more generic vs stating specifically. that bloom filter index is what was disabled.

vinothchandar · 2024-11-20T12:25:51Z

hudi-common/src/main/java/org/apache/hudi/metadata/MetadataPartitionType.java

@@ -312,6 +312,18 @@ private static void constructColumnStatsMetadataPayload(HoodieMetadataPayload pa
    }
  }

+  public static boolean isGenericIndex(String metadataPartitionPath) {


actually find this confusing. a generic index is sth that is either using a function to index or a secondary index?

not fixable in this PR i think

vinothchandar · 2024-11-20T12:27:33Z

...ark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/FunctionalIndexSupport.scala

+        Some(getCandidateFiles(indexDf, queryFilters, prunedFileNames))
+      } else if (indexDefinition.getIndexType.equals(HoodieTableMetadataUtil.PARTITION_NAME_BLOOM_FILTERS)) {
+        val prunedPartitionAndFileNames = getPrunedPartitionsAndFileNamesMap(prunedPartitionsAndFileSlices, includeLogFiles = true)
+        // TODO: we should get the list of keys to pass from queryFilters


don't we need this

vinothchandar · 2024-11-20T12:28:30Z

...ark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/FunctionalIndexSupport.scala

@@ -198,6 +209,44 @@ class FunctionalIndexSupport(spark: SparkSession,

    columnStatsRecords
  }
+
+  private def getPrunedPartitionsAndFileNamesMap(prunedPartitionsAndFileSlices: Seq[(Option[BaseHoodieTableFileIndex.PartitionPath], Seq[FileSlice])],


can we make these package private/protected to get some UTs on them?

vinothchandar · 2024-11-20T12:29:59Z

...asource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestSecondaryIndexPruning.scala

+        Seq("trip2", "rider-C")
+      )
+
+      if (true) {


@lokeshj1703 this was from my debugging.. :) please remove and cleanup.. Add calls to verifyQueryPredicate to verify query correctness

vinothchandar · 2024-11-20T12:30:09Z

...asource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestSecondaryIndexPruning.scala

+  }
+
+  @ParameterizedTest
+  @EnumSource(value = classOf[HoodieTableType])


same for this test

lokeshj1703 · 2024-11-21T13:47:51Z

I have manually verified that file pruning is working

codope

Looks good to me. One minor test comment.

.../hudi-spark/src/test/scala/org/apache/spark/sql/hudi/command/index/TestFunctionalIndex.scala

- Take the new tests in TestSecondaryIndexSupport, need to add query predicate verification asserts - See changes in both production files, around fixing one-off errors and wrong filepath/filename to build bloom index. - This needs to be done in a clean way - See changes in TestFunctionalIndex to unignore it and have it passing. take these changes. - Ignore my comments on memory management for now.

.../hudi-spark/src/test/scala/org/apache/spark/sql/hudi/command/index/TestFunctionalIndex.scala

hudi-bot · 2024-11-22T14:16:51Z

CI report:

c0f50b7 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

lokeshj1703 changed the title ~~[HUDI-8533] Bloom Index creation without function fails~~ [HUDI-8533] Bloom functional Index creation without function fails Nov 19, 2024

github-actions bot added the size:M PR with lines of changes in (100, 300] label Nov 19, 2024

vinothchandar reviewed Nov 19, 2024

View reviewed changes

github-actions bot added size:L PR with lines of changes in (300, 1000] and removed size:M PR with lines of changes in (100, 300] labels Nov 19, 2024

vinothchandar reviewed Nov 20, 2024

View reviewed changes

lokeshj1703 force-pushed the HUDI-8533 branch from 9d0de0c to 9a7c0e0 Compare November 21, 2024 13:46

codope reviewed Nov 21, 2024

View reviewed changes

.../hudi-spark/src/test/scala/org/apache/spark/sql/hudi/command/index/TestFunctionalIndex.scala Show resolved Hide resolved

lokeshj1703 and others added 8 commits November 22, 2024 12:31

[HUDI-8533] Bloom Index creation without function fails

c2cb3d4

Addendum changes

431a29d

func index bloom filter read support

e5bfa43

Add logic for query filter parsing

ae08e81

Address review comments

30c26ba

Fix checkstyle

7fd0194

Fix test failure

dfac1aa

codope reviewed Nov 22, 2024

View reviewed changes

.../hudi-spark/src/test/scala/org/apache/spark/sql/hudi/command/index/TestFunctionalIndex.scala Outdated Show resolved Hide resolved

codope force-pushed the HUDI-8533 branch from 3b19159 to 6274a5c Compare November 22, 2024 08:32

fix test

d563f40

codope force-pushed the HUDI-8533 branch from 6274a5c to d563f40 Compare November 22, 2024 10:08

disable spark.sql.defaultColumn.enabled for spark3.4

c0f50b7

codope approved these changes Nov 22, 2024

View reviewed changes

codope merged commit ae5833e into apache:master Nov 22, 2024
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-8533] Bloom functional Index creation without function fails #12290

[HUDI-8533] Bloom functional Index creation without function fails #12290

lokeshj1703 commented Nov 19, 2024

vinothchandar left a comment

vinothchandar Nov 19, 2024

vinothchandar Nov 19, 2024

vinothchandar left a comment

vinothchandar Nov 20, 2024

lokeshj1703 Nov 21, 2024

vinothchandar Nov 20, 2024

lokeshj1703 Nov 21, 2024

vinothchandar Nov 20, 2024

lokeshj1703 Nov 21, 2024

vinothchandar Nov 20, 2024

vinothchandar Nov 20, 2024

vinothchandar Nov 20, 2024

lokeshj1703 Nov 21, 2024

vinothchandar Nov 20, 2024

lokeshj1703 Nov 21, 2024

vinothchandar Nov 20, 2024

lokeshj1703 Nov 21, 2024

vinothchandar Nov 20, 2024

lokeshj1703 Nov 21, 2024

lokeshj1703 commented Nov 21, 2024

codope left a comment

hudi-bot commented Nov 22, 2024

		dataMetaClient.getTableConfig().setMetadataPartitionState(dataMetaClient, partitionPath, false);
		if (MetadataPartitionType.isGenericIndex(partitionPath)) {

[HUDI-8533] Bloom functional Index creation without function fails #12290

[HUDI-8533] Bloom functional Index creation without function fails #12290

Conversation

lokeshj1703 commented Nov 19, 2024

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokeshj1703 commented Nov 21, 2024

codope left a comment

Choose a reason for hiding this comment

hudi-bot commented Nov 22, 2024

CI report: