Add range filtering support for iceberg ingestion #15782

a2l007 · 2024-01-29T21:36:40Z

Description

Adds a new iceberg ingestion filter of type range which can be used to filter on ranges of column values. It supports lower and/or upper bounds with < , <= , > , and >=

Example spec:

               "icebergFilter": {
                  "type" : "range",
                  "filterColumn" : "price",
                  "upper": 50,
                  "lower": 45,
                  "lowerOpen": true
                  "upperOpen": false
                },

which is equivalent to 45 < price <= 50

This PR also fixes a dependency related issue which may be encountered while talking to Kerberos secured HDFS warehouses.

Release note

Adds a new iceberg ingestion filter of type range which can be used to filter on ranges of column values.

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

abhishekrb19

Looks good overall! I've left a few minor comments. Also, the docs static CI checks are failing.

abhishekrb19 · 2024-01-29T23:11:45Z

docs/ingestion/input-sources.md


+|Property|Description|Required|
+ |--------|-----------|---------|


Suggested change

|--------|-----------|---------|

|--------|-----------|---------|

abhishekrb19 · 2024-01-29T23:21:10Z

...uid-iceberg-extensions/src/main/java/org/apache/druid/iceberg/filter/IcebergRangeFilter.java

+      @JsonProperty("upperOpen") @Nullable Boolean upperOpen
+  )
+  {
+    Preconditions.checkNotNull(filterColumn, "You must specify a filter column on the range filter");


Consider using DruidException for these validation checks:

Suggested change

Preconditions.checkNotNull(filterColumn, "You must specify a filter column on the range filter");

if (filterColumn == null) {

throw InvalidInput.exception("filterColumn cannot be null.");

}

I can refactor this in a separate PR since the other filters also need to start using DruidException

Works for me, thanks!

abhishekrb19 · 2024-01-29T23:21:58Z

...uid-iceberg-extensions/src/main/java/org/apache/druid/iceberg/filter/IcebergRangeFilter.java

+  )
+  {
+    Preconditions.checkNotNull(filterColumn, "You must specify a filter column on the range filter");
+    Preconditions.checkArgument(lower != null || upper != null, "Both lower and upper bounds cannot be empty");


Suggested change

Preconditions.checkArgument(lower != null || upper != null, "Both lower and upper bounds cannot be empty");

if (lower == null && upper == null) {

throw InvalidInput.exception("Both lower and upper bounds cannot be null.");

}

Same comment as above.

abhishekrb19 · 2024-01-29T23:23:39Z

...uid-iceberg-extensions/src/main/java/org/apache/druid/iceberg/filter/IcebergRangeFilter.java

+  @Override
+  public Expression getFilterExpression()
+  {
+    List<Expression> expressions = new ArrayList<>(2);


nit: since one of lower and upper can be null

Suggested change

List<Expression> expressions = new ArrayList<>(2);

List<Expression> expressions = new ArrayList<>();

abhishekrb19 · 2024-01-29T23:24:08Z

...ib/druid-iceberg-extensions/src/main/java/org/apache/druid/iceberg/filter/IcebergFilter.java

-    @JsonSubTypes.Type(name = "or", value = IcebergOrFilter.class)
+    @JsonSubTypes.Type(name = "or", value = IcebergOrFilter.class),
+    @JsonSubTypes.Type(name = "range", value = IcebergRangeFilter.class)
+


Suggested change

abhishekrb19 · 2024-01-29T23:34:24Z

...uid-iceberg-extensions/src/main/java/org/apache/druid/iceberg/filter/IcebergRangeFilter.java

+    Preconditions.checkNotNull(filterColumn, "You must specify a filter column on the range filter");
+    Preconditions.checkArgument(lower != null || upper != null, "Both lower and upper bounds cannot be empty");
+    this.filterColumn = filterColumn;
+    this.lowerOpen = lowerOpen != null ? lowerOpen : false;


nit: should we also add checks to validate if lowerOpen is specified if lower is null as it doesn't make sense? Ditto for upperOpen

It could be checked, but are you suggesting that the ingest should fail in that scenario?

I would think so. I will leave it up to you though if the stricter validation is required

abhishekrb19 · 2024-01-29T23:37:02Z

docs/ingestion/input-sources.md

+|filterColumn|The column name from the iceberg table schema based on which range filtering needs to happen.|yes|
+|lower|Lower bound value to match.|no. At least one of `lower` or `upper` must not be null.|
+|upper|Upper bound value to match. |no. At least one of `lower` or `upper` must not be null.|
+|lowerOpen|Boolean indicating if lower bound is open in the interval of values defined by the range (">" instead of ">="). |no|


Would also be good to specify the defaults explicitly via a separate column or next to the "no" in the required column

abhishekrb19

+1 after CI. Thanks, @a2l007!

* Add range filtering support for iceberg ingestion * Docs formatting * Spelling

Add range filtering support for iceberg ingestion

54cb2a9

github-actions bot added Area - Documentation Area - Dependencies labels Jan 29, 2024

abhishekrb19 reviewed Jan 29, 2024

View reviewed changes

a2l007 added 2 commits February 1, 2024 11:51

Merge branch 'master' of github.com:apache/druid into rangefilter

2f8c6a6

Docs formatting

24b51c8

abhishekrb19 approved these changes Feb 1, 2024

View reviewed changes

Spelling

c899562

abhishekrb19 merged commit 2e46a98 into apache:master Feb 2, 2024
83 checks passed

abhishekrb19 added this to the 29.0.0 milestone Feb 2, 2024

abhishekrb19 pushed a commit to abhishekrb19/incubator-druid that referenced this pull request Feb 2, 2024

Add range filtering support for iceberg ingestion (apache#15782)

085f76d

* Add range filtering support for iceberg ingestion * Docs formatting * Spelling

abhishekrb19 mentioned this pull request Feb 2, 2024

[Backport] Add range filtering support for Iceberg ingestion and fixup typo in example #15824

Merged

abhishekrb19 pushed a commit that referenced this pull request Feb 2, 2024

Add range filtering support for iceberg ingestion (#15782)

b44bd34

* Add range filtering support for iceberg ingestion * Docs formatting * Spelling

LakshSingla mentioned this pull request Feb 13, 2024

[DRAFT] 29.0.0 release notes #15896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add range filtering support for iceberg ingestion #15782

Add range filtering support for iceberg ingestion #15782

a2l007 commented Jan 29, 2024

abhishekrb19 left a comment •

edited

Loading

abhishekrb19 Jan 29, 2024

abhishekrb19 Jan 29, 2024

a2l007 Feb 1, 2024

abhishekrb19 Feb 1, 2024

abhishekrb19 Jan 29, 2024

a2l007 Feb 1, 2024

abhishekrb19 Jan 29, 2024

abhishekrb19 Jan 29, 2024

abhishekrb19 Jan 29, 2024

a2l007 Feb 1, 2024

abhishekrb19 Feb 1, 2024

abhishekrb19 Jan 29, 2024

abhishekrb19 left a comment


		\|Property\|Description\|Required\|
		\|--------\|-----------\|---------\|

-    Preconditions.checkNotNull(filterColumn, "You must specify a filter column on the range filter");
+    if (filterColumn == null) {
+      throw InvalidInput.exception("filterColumn cannot be null.");
+    }

-    Preconditions.checkArgument(lower != null || upper != null, "Both lower and upper bounds cannot be empty");
+    if (lower == null && upper == null) {
+      throw InvalidInput.exception("Both lower and upper bounds cannot be null.");
+    }

	List<Expression> expressions = new ArrayList<>(2);
	List<Expression> expressions = new ArrayList<>();

Add range filtering support for iceberg ingestion #15782

Add range filtering support for iceberg ingestion #15782

Conversation

a2l007 commented Jan 29, 2024

Description

Release note

abhishekrb19 left a comment • 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

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

abhishekrb19 left a comment

Choose a reason for hiding this comment

abhishekrb19 left a comment •

edited

Loading