ORC-1310: Allowlist Support for plugin filter #1314

deshanxiao · 2022-11-14T11:40:30Z

What changes were proposed in this pull request?

This PR is aimed to add allowlist support for plugin filter.

Why are the changes needed?

ServiceLoader will load all the interfaces in current classpath. When we have two implementations of PluginFilterService, Both of them will be loaded as plugin filters. This PR will provide a configuration "orc.filter.plugin.allowlist". ORC reader will only load the class which is in the allowlist when the configuration is not null.

How was this patch tested?

UT

dongjoon-hyun

Thank you for making a PR, however, Apache ORC intentionally aims to avoid non-inclusive languages like whitelist. Could you revise your PR to use more conscious language?

FYI, we are checking like the following.

https://clc.diversity.apache.org/analysis.html?project=orc.git

dongjoon-hyun · 2022-11-14T17:09:25Z

cc @williamhyun and @wgtmac , too

williamhyun · 2022-11-15T04:42:00Z

+1 for @dongjoon-hyun's comment

deshanxiao · 2022-11-15T04:53:13Z

Sorry @dongjoon-hyun , will update it. Thanks~

deshanxiao · 2022-11-15T05:15:56Z

@dongjoon-hyun I have updated the jira and the pull request.

dongjoon-hyun · 2022-11-16T16:41:47Z

Thank you for update, @deshanxiao .

dongjoon-hyun · 2022-11-16T16:44:12Z

java/core/src/java/org/apache/orc/OrcConf.java

+
+  PLUGIN_FILTER_ALLOWLIST("orc.filter.plugin.allowlist",
+                          "orc.filter.plugin.allowlist",
+                          "",


It looks like a breaking change if this mean that we ban every plugins by default.

dongjoon-hyun

It looks like a breaking change to the existing production, @deshanxiao . Is the intention of this PR correct?

deshanxiao · 2022-11-16T17:05:38Z

It looks like a breaking change to the existing production, @deshanxiao . Is the intention of this PR correct?

Actually if orc.filter.plugin.allowlist is empty. Everything will the same as before. I think it will be better if change the default value to *.

dongjoon-hyun · 2022-11-16T18:24:05Z

java/core/src/java/org/apache/orc/impl/filter/FilterFactory.java

+   *                  loaded if it is not in allowList.
+   */
+  static void deleteFilterNotInAllowList(List<BatchFilter> filters, List<String> allowList) {
+    if (allowList != null && allowList.size() > 0) {


Given the function name, deleteFilterNotInAllowList and description, this logic looks wrong to me.

dongjoon-hyun · 2022-11-16T18:25:18Z

java/core/src/test/org/apache/orc/impl/filter/TestPluginFilterService.java

@@ -56,4 +60,24 @@ public void testErrorCreatingFilter() {
  public void testMissingFilter() {
    assertTrue(FilterFactory.findPluginFilters("file://db/table11/file1", conf).isEmpty());
  }
+
+  @Test
+  public void testAllowListFilter() {


Please add additional test cases explicitly and revise the logic.

testAllowListFilterAllowAll

testAllowListFilterDisallowAll

dongjoon-hyun · 2022-11-16T18:26:41Z

java/core/src/test/org/apache/orc/impl/filter/TestPluginFilterService.java

+    assertEquals(1, pluginFilters.size());
+    // Empty allowlist.
+    List<String> emptyAllowListNotHit = new ArrayList<>();
+    FilterFactory.deleteFilterNotInAllowList(pluginFilters, emptyAllowListNotHit);


// Empty allowlist. sounds misleading. We had better clarify what empty allowlist means.

dongjoon-hyun · 2022-11-21T03:56:28Z

java/core/src/java/org/apache/orc/OrcConf.java

+  PLUGIN_FILTER_ALLOWLIST("orc.filter.plugin.allowlist",
+                          "orc.filter.plugin.allowlist",
+                          "*",
+                          "A comma-separated list of allowlist classes. It is used to specify the "


It looks like recursive definition. We cannot use allowlist in the definition sentence of allowlist. Maybe, the following is better?

- A comma-separated list of allowlist classes + A list of comma-separated class names

pavibhai · 2022-11-28T16:04:52Z

Thanks @deshanxiao for raising this. Can you please help me understand the use a little better? Is it something like

Is this more like I have started a spark session with a set of dependencies (that in this case might include plugin filter classes). However I might not want to engage these filters for some queries while I might use it for other queries from the same session.

deshanxiao · 2022-11-29T04:00:37Z

@pavibhai Yes, you are right. In fact, a spark query does not want to use all plugin filter classes because some plugin filter classes are used by other queries. Now, according to the mechanism of ServiceLoader , all plugin filter classes will be loaded into any queies.
With this PR we can choose the plugin filter classes we want to use.

java/core/src/java/org/apache/orc/OrcConf.java

java/core/src/java/org/apache/orc/impl/filter/FilterFactory.java

pavibhai · 2022-11-29T15:16:43Z

java/core/src/java/org/apache/orc/impl/filter/FilterFactory.java

-      if (!pluginFilters.isEmpty()) {
-        LOG.debug("Added plugin filters {} to the read", pluginFilters);
-        filters.addAll(pluginFilters);
+      List<BatchFilter> allowFilters =


Maybe we can move this logic into findPluginFilters?

Please rename this to pluginFilters to minize the diff.

java/core/src/java/org/apache/orc/impl/filter/FilterFactory.java

java/core/src/test/org/apache/orc/impl/filter/TestPluginFilterService.java

deshanxiao · 2022-12-01T02:51:58Z

@pavibhai Thanks for your suggestions!

deshanxiao · 2022-12-05T02:35:59Z

Hi @pavibhai, I have updated all the comments, please help to see if you have time, thanks~

pavibhai · 2022-12-06T00:05:11Z

Hi @pavibhai, I have updated all the comments, please help to see if you have time, thanks~

Sure. I shall take a look first thing tomorrow morning

pavibhai · 2022-12-06T18:48:27Z

Hi @pavibhai, I have updated all the comments, please help to see if you have time, thanks~

Looks good. I have no further comments on this.

dongjoon-hyun · 2022-12-07T10:57:26Z

java/core/src/java/org/apache/orc/OrcConf.java

+      return new ArrayList<>();
+    }
+    return Arrays.asList(confStr.split(","));
+  }


If you don't mind, could you open a new JIRA and PR for this utility method, @deshanxiao ?

And, it would be great to remove white space and empty strings after split. For example, a,,b,c, ,d,?

I saw your JIRA and have been waiting for your PR, @deshanxiao :)

I merged #1337 .

dongjoon-hyun

#1338 is merged. Could you rebase to main branch and resolve the conflicts?

deshanxiao · 2022-12-12T01:20:04Z

@dongjoon-hyun I have resolved the conflicts, thank you!

dongjoon-hyun · 2022-12-12T01:29:34Z

Thank you for update, @deshanxiao .

dongjoon-hyun · 2022-12-12T01:52:43Z

java/core/src/java/org/apache/orc/impl/filter/FilterFactory.java

+   * @param filters whole BatchFilter list we load from class path.
+   * @param allowList a Class-Name list that we want to load in.
+   */
+  static List<BatchFilter> getAllowedFilters(List<BatchFilter> filters, List<String> allowList) {


Is it open for test? I'd recommend to switch to private static?

dongjoon-hyun

I opened a PR to you, @deshanxiao .

https://github.com/deshanxiao/orc/pull/1/files

Convert to `private static` method

deshanxiao · 2022-12-12T03:54:35Z

Thank you, I merge it.

dongjoon-hyun

+1, LGTM.

dongjoon-hyun · 2022-12-13T00:47:52Z

Merged to main for Apache ORC 1.9.0. Thank you, @deshanxiao and @pavibhai .

### What changes were proposed in this pull request? This PR is aimed to add allowlist support for plugin filter. ### Why are the changes needed? **ServiceLoader** will load all the interfaces in current classpath. When we have two implementations of PluginFilterService, Both of them will be loaded as plugin filters. This PR will provide a configuration "orc.filter.plugin.allowlist". ORC reader will only load the class which is in the allowlist when the configuration is not null. ### How was this patch tested? UT Closes apache#1314 from deshanxiao/deshan/1310. Lead-authored-by: deshanxiao <deshanxiao@microsoft.com> Co-authored-by: Deshan Xiao <deshanxiao@microsoft.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Co-authored-by: Deshan Xiao <42019462+deshanxiao@users.noreply.github.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

deshanxiao added 5 commits November 14, 2022 18:49

init

0baa001

add test

abea4cd

fix

650efcb

fix

5f87de2

fix

0356fda

github-actions bot added the JAVA label Nov 14, 2022

update

9c81525

dongjoon-hyun requested changes Nov 14, 2022

View reviewed changes

update

49bc4e3

deshanxiao changed the title ~~ORC-1310: Whitelist Support for plugin filter~~ ORC-1310: Allowlist Support for plugin filter Nov 15, 2022

dongjoon-hyun reviewed Nov 16, 2022

View reviewed changes

dongjoon-hyun requested changes Nov 16, 2022

View reviewed changes

dongjoon-hyun reviewed Nov 16, 2022

View reviewed changes

deshanxiao added 3 commits November 17, 2022 18:53

update

793c596

fix-func-name

12ca809

fix-test

a25a118

deshanxiao requested a review from dongjoon-hyun November 18, 2022 09:14

dongjoon-hyun reviewed Nov 21, 2022

View reviewed changes

update

bff5433

deshanxiao requested a review from dongjoon-hyun November 28, 2022 02:39

pavibhai reviewed Nov 29, 2022

View reviewed changes

deshanxiao added 2 commits December 1, 2022 10:55

Merge branch 'main' into deshan/1310

a9277d0

fix comments

9783d61

dongjoon-hyun reviewed Dec 7, 2022

View reviewed changes

dongjoon-hyun reviewed Dec 10, 2022

View reviewed changes

merge main

de76263

dongjoon-hyun reviewed Dec 12, 2022

View reviewed changes

Revise

7df1db2

dongjoon-hyun requested changes Dec 12, 2022

View reviewed changes

Merge pull request #1 from dongjoon-hyun/ORC-1310

745b0bd

Convert to `private static` method

rename variable

b38e45d

dongjoon-hyun approved these changes Dec 13, 2022

View reviewed changes

dongjoon-hyun added this to the 1.9.0 milestone Dec 13, 2022

dongjoon-hyun closed this in becf5a5 Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1310: Allowlist Support for plugin filter #1314

ORC-1310: Allowlist Support for plugin filter #1314

deshanxiao commented Nov 14, 2022 •

edited

dongjoon-hyun left a comment •

edited

dongjoon-hyun commented Nov 14, 2022

williamhyun commented Nov 15, 2022

deshanxiao commented Nov 15, 2022

deshanxiao commented Nov 15, 2022

dongjoon-hyun commented Nov 16, 2022

dongjoon-hyun Nov 16, 2022

dongjoon-hyun left a comment

deshanxiao commented Nov 16, 2022 •

edited

dongjoon-hyun Nov 16, 2022

dongjoon-hyun Nov 16, 2022

dongjoon-hyun Nov 16, 2022

dongjoon-hyun Nov 21, 2022

pavibhai commented Nov 28, 2022

deshanxiao commented Nov 29, 2022 •

edited

pavibhai Nov 29, 2022

dongjoon-hyun Dec 12, 2022

deshanxiao commented Dec 1, 2022

deshanxiao commented Dec 5, 2022

pavibhai commented Dec 6, 2022

pavibhai commented Dec 6, 2022

dongjoon-hyun Dec 7, 2022 •

edited

dongjoon-hyun Dec 9, 2022

dongjoon-hyun Dec 10, 2022

dongjoon-hyun left a comment

deshanxiao commented Dec 12, 2022

dongjoon-hyun commented Dec 12, 2022

dongjoon-hyun Dec 12, 2022

dongjoon-hyun left a comment

deshanxiao commented Dec 12, 2022

dongjoon-hyun left a comment

dongjoon-hyun commented Dec 13, 2022

ORC-1310: Allowlist Support for plugin filter #1314

ORC-1310: Allowlist Support for plugin filter #1314

Conversation

deshanxiao commented Nov 14, 2022 • edited

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

dongjoon-hyun left a comment • edited

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 14, 2022

williamhyun commented Nov 15, 2022

deshanxiao commented Nov 15, 2022

deshanxiao commented Nov 15, 2022

dongjoon-hyun commented Nov 16, 2022

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

deshanxiao commented Nov 16, 2022 • edited

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

pavibhai commented Nov 28, 2022

deshanxiao commented Nov 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshanxiao commented Dec 1, 2022

deshanxiao commented Dec 5, 2022

pavibhai commented Dec 6, 2022

pavibhai commented Dec 6, 2022

dongjoon-hyun Dec 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

deshanxiao commented Dec 12, 2022

dongjoon-hyun commented Dec 12, 2022

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

deshanxiao commented Dec 12, 2022

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 13, 2022

deshanxiao commented Nov 14, 2022 •

edited

dongjoon-hyun left a comment •

edited

deshanxiao commented Nov 16, 2022 •

edited

deshanxiao commented Nov 29, 2022 •

edited

dongjoon-hyun Dec 7, 2022 •

edited