[native] Enable hive.allow-drop-table in NativeQueryRunner

[pdabre12]

Allow NativeQueryRunner to drop tables. We need this for writing CTAS tests via the NativeQueryRunner.

Resolves: #21659

[mbasmanova]

@pdabre12 Are you new to Presto? Please, review contributing guildelines and make sure to create independently buildable commits with nice commit messages that follow the guidelines.

[pdabre12]

@mbasmanova Sorry for the inconvenience, I missed to tag this PR as a draft, it's still in progress. I will clean it up and make sure its according to the community guidelines before its ready for review.

[mbasmanova]

@pdabre12 Sounds good. Would you also create a GitHub issue to describe the problem you are solving and the approach you use to solve it?

[pdabre12]

@mbasmanova Yes, I will.

[pdabre12]

@majetideepak Please review.

[majetideepak]

The commit must have the [native] prefix.
See PR guidelines here https://github.com/prestodb/presto/tree/master/presto-native-execution#create-pull-request

[majetideepak]

@pdabre12 Is there a reason to not use the builder()?

[majetideepak]

Why do we need this change?

[pdabre12]

The reason I did not use the builder() is because I wanted to make use of the putIfAbsent() method available on HashMap and as far as I know we cannot create an HashMap with a builder().

[pdabre12]

This change is needed because under the hood while creating a queryRunner using PrestoSparkNativeQueryRunnerUtils , the native worker properties (hiveProperties above) are added to the properties.

Not adding the putIfAbsent() check there throws an error : duplicate key values for hive.allow-drop-table .

[majetideepak]

Can we simply remove .put("hive.allow-drop-table", "true") below to avoid the duplicate key issue?

[pdabre12]

No we cannot because the PrestoSparkNativeQueryRunnerUtils uses the PrestoSparkQueryRunner present in presto-spark-base and PrestoSparkQueryRunner is called separately too while testing presto-spark-base operations. Removing these properties from here results in an error while running those tests.

[majetideepak]

Then likely getNativeWorkerHiveProperties is not the right place to add this.
Can we add inside at presto-hive/src/test/java/com/facebook/presto/hive/HiveQueryRunner.java:225?

[aditi-pandit]

@pdabre12 : Any reason we have not added the rest of the properties viz. hive.allow-rename-(table)column, hive.allow-add-column, hive.allow-drop-column to NativeQueryRunnerUtils.getNativeWorkerHiveProperties() function above ?

[majetideepak]

I guess the need was only for hive.allow-drop-table. But good point. We could add the remaining and remove them from here.

[pdabre12]

Yup , the need was only for hive.allow-drop-table.
My plan was to add the other properties once this refactor was accepted.

[majetideepak]

@pdabre12 thanks for iterating on this. My goal is to avoid changes to the non-native code paths since some of the APIs are reused across different contexts.
The diff below limits the changes to native only. Will this work?

--- a/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
+++ b/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
@@ -151,6 +151,11 @@ public class PrestoNativeQueryRunnerUtils
             boolean addStorageFormatToPath)
             throws Exception
     {
+        ImmutableMap.Builder<String, String> hivePropertiesBuilder = new ImmutableMap.Builder<>();
+        hivePropertiesBuilder
+                .putAll(getNativeWorkerHiveProperties(storageFormat))
+                .put("hive.allow-drop-table", "true");
+
         // Make query runner with external workers for tests
         return HiveQueryRunner.createQueryRunner(
                 ImmutableList.of(),
@@ -162,7 +167,7 @@ public class PrestoNativeQueryRunnerUtils
                         .build(),
                 ImmutableMap.of(),
                 "legacy",
-                getNativeWorkerHiveProperties(storageFormat),
+                hivePropertiesBuilder.build(),
                 workerCount,
                 Optional.of(Paths.get(addStorageFormatToPath ? dataDirectory + "/" + storageFormat : dataDirectory)),
                 Optional.of((workerIndex, discoveryUri) -> {

[majetideepak]

Thanks, @pdabre12

[majetideepak]

@mbasmanova can you take another look? Thanks.

[mbasmanova]

Why is this not part of getNativeWorkerHiveProperties ?

Can you update PR description to explain why you are making this change?

[aditi-pandit]

+1. Agree with @mbasmanova. I too feel this should be a part of getNativeWorkerHiveProperties(). Several people look at that function when setting up their catalogs.

[majetideepak]

We tried that. getNativeWorkerHiveProperties() is used on the Native Spark side as well and it conflicted due to duplicate config.
The diff would look like 2affa78

You will see that on the Spark side, the corresponding config is set inside the PrestoSparkQueryRunner.
We followed the existing pattern and avoided risky refactoring.

[majetideepak]

@mbasmanova I updated the description. Thanks.

[majetideepak]

Since these classes are used for both Native and Java paths, we should have native-only, java-only, and common config.
hive.allow-drop-table is a common config.

[majetideepak]

If the above classification makes sense, we could refactor the existing config in a separate PR.

[majetideepak]

I guess there is also Spark config.

[mbasmanova]

Do we know why PrestoSparkQueryRunner sets this by default, but Hive query runner doesn't?

CC: @arhimondr

[mbasmanova]

Allow NativeQueryRunner to drop tables. We need this for writing CTAS tests via the NativeQueryRunner.

@majetideepak How does this work for native Presto-on-Spark?

[majetideepak]

How does this work for native Presto-on-Spark?

@mbasmanova I see that it also invokes the PrestoSparkQueryRunner inside which the hive.allow-drop-table property is set.
https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/spark/PrestoSparkNativeQueryRunnerUtils.java#L138

[majetideepak]

The createJavaQueryRunner sets it here
https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java#L125
We followed the same for createNativeQueryRunner.

[pdabre12]

How does this work for native Presto-on-Spark?

@mbasmanova I see that it also invokes the PrestoSparkQueryRunner inside which the hive.allow-drop-table property is set. https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/spark/PrestoSparkNativeQueryRunnerUtils.java#L138

The hive.allow-drop-table property is set in PrestoSparkQueryRunner here: https://github.com/prestodb/presto/blob/master/presto-spark-base/src/test/java/com/facebook/presto/spark/PrestoSparkQueryRunner.java#L382

[mbasmanova]

The solution in createJavaQueryRunner looks the best. Let's do the same for native query runner and unify Prestissimo and Presto-on-Spark.

        if ("legacy".equals(security)) {
            hivePropertiesBuilder.put("hive.allow-drop-table", "true");
        }

[majetideepak]

@mbasmanova the solution in this PR is what createJavaQueryRunner does. Except that security is not an argument to the createNativeQueryRunner and we always add hive.allow-drop-table.

[majetideepak]

@mbasmanova can you approve this change so that we can unblock the CTAS tests?
The current change aligns with createJavaQueryRunner.
There is some historical context here that we should understand and refactor the configs in a separate PR.

[mbasmanova]

@majetideepak Deepak, I'm confused because I believe we already have CTAS tests. Is this not the case? What's blocking us from adding more?

[mbasmanova]

nit: maybe

ImmutableMap.<String, String>builder()
   .putAll(getNativeWorkerHiveProperties(storageFormat))
   .put("hive.allow-drop-table", "true")
   .build();

and let's add comment to explain that we need hive.allow-drop-table=true because we use "legacy" security.

[pdabre12]

@mbasmanova Thanks for the review, added the comment .

[majetideepak]

I'm confused because I believe we already have CTAS tests. Is this not the case? What's blocking us from adding more?

The current native tests are set up such that the JavaQueryRunner writes the tables and the NativeQueryRunner reads the tables.
The new CTAS tests that we are planning to add would be the opposite. The NativeQueryRunner would write the tables and JavaQueryRunner would read the tables.
See issue #20848

[mbasmanova]

Got it. Thanks.

Please, add a comment to explain why you are adding that property and use

ImmutableMap.<String, String>builder() 
   .putAll(getNativeWorkerHiveProperties(storageFormat)) 
   .put("hive.allow-drop-table", "true") 
   .build();

[majetideepak]

To add more context, some existing test CTAS APIs drop tables. So we need the queryrunner to be able to drop a table.

    protected void assertCreateTableAsSelect(Session session, String table, @Language("SQL") String query, @Language("SQL") String expectedQuery, @Language("SQL") String rowCountQuery)
    {
        assertUpdate(session, "CREATE TABLE " + table + " AS " + query, rowCountQuery);
        assertQuery(session, "SELECT * FROM " + table, expectedQuery);
        assertUpdate(session, "DROP TABLE " + table);

        assertFalse(getQueryRunner().tableExists(session, table));
    }