[CARBONDATA-4091] support prestosql 333 integartion with carbon #4034

ajantha-bhat · 2020-12-02T07:38:28Z

Why is this PR needed?

Currently carbondata is integrated with presto-sql 316, which is 1.5 years older.
There are many good features and optimization that came into presto like dynamic filtering, Rubix data cache and some performance improvements.

It is always good to use latest version, latest version is presto-sql 348.
But jumping from 316 to 348 will be too many changes.
So, to utilize these new features and based on customer demand, I suggest to upgrade presto-sql to 333 version.
Later it will be again upgraded to more latest version in few months.

Note:
This is a plain integration to support all existing features of presto316, deep integration to support new features like dynamic filtering, Rubix cache will be handled in another PR.

What changes were proposed in this PR?

Adapt to the new hive adapter changes like some constructor changes, Made a carbonDataConnector to support CarbonDataHandleResolver
Java 11 removed ConstructorAccessor class, so using unsafe class for reflection. (presto333 depend on java 11 for runtime)
POM changes to support presto333

Note: JAVA 11 environment is needed for running presto333 with carbon and also need add this jvm property "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED"

Does this PR introduce any user interface change?

No

Is any new testcase added?

No

CarbonDataQA2 · 2020-12-02T09:24:36Z

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/5017/

CarbonDataQA2 · 2020-12-02T09:36:03Z

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3261/

CarbonDataQA2 · 2020-12-08T10:02:36Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3349/

CarbonDataQA2 · 2020-12-08T10:02:47Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5111/

CarbonDataQA2 · 2020-12-08T18:51:54Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5119/

CarbonDataQA2 · 2020-12-08T18:59:54Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3357/

CarbonDataQA2 · 2020-12-09T09:10:06Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5123/

CarbonDataQA2 · 2020-12-09T09:13:23Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3361/

CarbonDataQA2 · 2020-12-18T08:08:22Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5201/

CarbonDataQA2 · 2020-12-18T08:10:16Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3441/

CarbonDataQA2 · 2020-12-18T10:15:51Z

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3300/

CarbonDataQA2 · 2020-12-18T13:43:11Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3450/

CarbonDataQA2 · 2020-12-18T13:49:29Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5210/

CarbonDataQA2 · 2020-12-30T09:42:20Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5264/

CarbonDataQA2 · 2020-12-30T09:43:01Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3503/

akashrn5 · 2021-01-08T05:37:40Z

integration/presto/pom.xml

@@ -294,7 +294,7 @@
    <dependency>
      <groupId>io.airlift</groupId>
      <artifactId>json</artifactId>
-      <version>0.144</version>
+      <version>0.193</version>


just a suggestion, can you put this version in a variable in POM, both airlift and jackson, so that in future while changing version it will be easier and chances of missing will be less.

If a different maven profile needs different version, then it makes sense to add a variable, here it is not needed. Also, each is a different artifact.

akashrn5 · 2021-01-08T06:13:17Z

integration/presto/src/main/prestosql/org/apache/carbondata/presto/CarbonDataMetaData.java

+      HiveStatisticsProvider hiveStatisticsProvider,
+      AccessControlMetadata accessControlMetadata) {
+    super(catalogName, metastore, hdfsEnvironment, partitionManager, timeZone,
+        allowCorruptWritesForTesting, writesToNonManagedTablesEnabled,


writesToNonManagedTablesEnabled make this as true like before as it was failing for insert scenarios when impletemented write feature. Or is the default value changed now?

yes, cluster I face some issue in the insert. Still debugging

Insert has passed now.

presto:aj333> select * from t1;
name

aj
ab
(2 rows)

Query 20210113_015933_00005_6sfgc, FINISHED, 1 node
Splits: 18 total, 18 done (100.00%)
0:04 [2 rows, 22B] [0 rows/s, 5B/s]

presto:aj333> insert into t1 values('junk');
INSERT: 1 row

Query 20210113_015940_00006_6sfgc, FINISHED, 1 node
Splits: 35 total, 35 done (100.00%)
0:04 [0 rows, 0B] [0 rows/s, 0B/s]

presto:aj333> select * from t1;
name

junk
aj
ab
(3 rows)

Query 20210113_015951_00007_6sfgc, FINISHED, 1 node
Splits: 19 total, 19 done (100.00%)
0:00 [3 rows, 35B] [11 rows/s, 137B/s]

I reverted back to writesToNonManagedTablesEnabled instead of hardcoded true value as cluster test is passed after reverting also.

akashrn5 · 2021-01-08T06:16:12Z

...ation/presto/src/main/prestosql/org/apache/carbondata/presto/CarbonDataPageSinkProvider.java

+            handle.getPageSinkMetadata(),
+            new HiveMetastoreClosure(
+                memoizeMetastore(metastore, perTransactionMetastoreCacheMaximumSize)),
+            new HiveIdentity(session)),


from line 151 to 155, please reformat the code

akashrn5 · 2021-01-08T06:21:48Z

integration/presto/src/main/prestosql/org/apache/carbondata/presto/CarbonMetadataFactory.java

 public class CarbonMetadataFactory extends HiveMetadataFactory {

  private static final Logger log = Logger.get(HiveMetadataFactory.class);
  private final boolean allowCorruptWritesForTesting;
  private final boolean skipDeletionForAlter;
  private final boolean skipTargetCleanupOnRollback;
-  private final boolean writesToNonManagedTablesEnabled = true;
+  private final boolean writesToNonManagedTablesEnabled;


could you please confirm that once after this changes, the insert is working fine in cluster test?

yes, cluster it works fine without hardcoding writesToNonManagedTablesEnabled = true

akashrn5 · 2021-01-08T06:28:25Z

...ation/presto/src/main/prestosql/org/apache/carbondata/presto/CarbondataConnectorFactory.java

-  public CarbondataConnectorFactory(String connectorName, ClassLoader classLoader) {
-    super(connectorName, classLoader, Optional.empty());
-    this.classLoader = requireNonNull(classLoader, "classLoader is null");
+  public CarbondataConnectorFactory(String name) {


Suggested change

public CarbondataConnectorFactory(String name) {

public CarbondataConnectorFactory(String connectorName) {

akashrn5 · 2021-01-08T06:39:21Z

...ation/presto/src/main/prestosql/org/apache/carbondata/presto/CarbondataConnectorFactory.java

      throw new RuntimeException(e);
    }
  }

  /**
   * Set the Carbon format enum to HiveStorageFormat, its a hack but for time being it is best
   * choice to avoid lot of code change.
+   *
+   * @throws Exception


can revert this, as no specific info

it is good to have proper java doc. Also it doesn't have any params, but stills throws exception. so recording in java doc. you can also observe similar in old code

akashrn5 · 2021-01-08T06:57:33Z

integration/presto/src/main/prestosql/org/apache/carbondata/presto/CarbondataModule.java

  }

+  public static final class TypeDeserializer


do we need this? as its already present in HiveModule class and accessible right?

yes, can use it.

akashrn5 · 2021-01-08T07:00:49Z

...ion/presto/src/main/prestosql/org/apache/carbondata/presto/CarbondataPageSourceProvider.java

    }
+    // TODO: check and use dynamicFilter in CarbondataPageSource


if possible, can you create jira for this and add the jira in TODO?

https://issues.apache.org/jira/browse/CARBONDATA-4103

akashrn5 · 2021-01-08T07:05:43Z

...esto/src/main/prestosql/org/apache/carbondata/presto/InternalCarbonDataConnectorFactory.java

+import static io.airlift.configuration.ConfigBinder.configBinder;
+import static java.util.Objects.requireNonNull;
+
+public final class InternalCarbonDataConnectorFactory {


please add a class level comment

akashrn5 · 2021-01-08T07:08:38Z

pom.xml

@@ -780,9 +780,9 @@
        <activeByDefault>true</activeByDefault>
      </activation>
      <properties>
-        <presto.version>316</presto.version>
+        <presto.version>333</presto.version>


similar to one of the able comment, can we design this a variable and use everywhere?

<presto.version> is a variable itself.

CarbonDataQA2 · 2021-01-08T16:28:47Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5291/

CarbonDataQA2 · 2021-01-08T16:29:29Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3531/

CarbonDataQA2 · 2021-01-13T04:29:27Z

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5304/

CarbonDataQA2 · 2021-01-13T04:30:25Z

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3544/

ajantha-bhat · 2021-04-06T02:26:12Z

retest this please

CarbonDataQA2 · 2021-04-06T05:24:52Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5126/

CarbonDataQA2 · 2021-04-06T05:29:20Z

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3375/

ydvpankaj99 · 2021-04-06T07:10:41Z

retest this please

CarbonDataQA2 · 2021-04-06T08:53:22Z

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5132/

CarbonDataQA2 · 2021-04-06T09:00:21Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3381/

CarbonDataQA2 · 2021-05-12T14:28:13Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3605/

CarbonDataQA2 · 2021-05-12T14:33:23Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5350/

jackylk · 2021-05-18T06:19:51Z

LGTM

CarbonDataQA2 · 2021-08-05T08:19:30Z

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5815/

CarbonDataQA2 · 2021-08-05T09:58:46Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4071/

CarbonDataQA2 · 2021-08-05T10:13:25Z

Build Success with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/218/

ajantha-bhat · 2021-08-06T06:52:40Z

retest this please

CarbonDataQA2 · 2021-08-06T09:18:18Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4075/

CarbonDataQA2 · 2021-08-06T09:19:26Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5820/

CarbonDataQA2 · 2021-08-06T09:30:22Z

Build Failed with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/223/

brijoobopanna · 2021-08-10T05:36:00Z

retest this please

CarbonDataQA2 · 2021-08-10T07:47:38Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5836/

CarbonDataQA2 · 2021-08-10T08:01:57Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4092/

CarbonDataQA2 · 2021-08-10T08:02:37Z

Build Success with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/240/

akashrn5 · 2021-08-10T14:40:11Z

LGTM

Indhumathi27 · 2021-08-11T05:11:42Z

retest this please

CarbonDataQA2 · 2021-08-11T08:03:03Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/4097/

CarbonDataQA2 · 2021-08-11T08:08:11Z

Build Success with Spark 3.1, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_3.1/245/

CarbonDataQA2 · 2021-08-11T08:09:41Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5841/

ajantha-bhat force-pushed the presto333 branch from 7f665e5 to 5be2097 Compare December 8, 2020 08:18

ajantha-bhat force-pushed the presto333 branch 2 times, most recently from 8fc6686 to 5be2097 Compare December 8, 2020 17:12

ajantha-bhat force-pushed the presto333 branch from 9624fb3 to c294078 Compare December 9, 2020 07:30

ajantha-bhat force-pushed the presto333 branch from c294078 to 2fc5e1a Compare December 18, 2020 06:29

ajantha-bhat force-pushed the presto333 branch from 2fc5e1a to 7c3505d Compare December 18, 2020 11:51

ajantha-bhat changed the title ~~[WIP] support prestosql 333~~ [CARBONDATA-4091] support prestosql 333 integartion with carbon Dec 18, 2020

ajantha-bhat force-pushed the presto333 branch from 7c3505d to cf9605b Compare December 30, 2020 08:10

akashrn5 reviewed Jan 8, 2021

View reviewed changes

ajantha-bhat force-pushed the presto333 branch from cf9605b to 2609395 Compare January 8, 2021 14:55

ajantha-bhat force-pushed the presto333 branch from 2609395 to 8a820e5 Compare January 13, 2021 02:59

ajantha-bhat force-pushed the presto333 branch from aae0601 to 3c44214 Compare May 12, 2021 12:49

support presto 333

c8a36c2

ajantha-bhat force-pushed the presto333 branch from 3c44214 to c8a36c2 Compare August 5, 2021 07:40

asfgit closed this in 1ccf295 Aug 11, 2021

	public CarbondataConnectorFactory(String name) {
	public CarbondataConnectorFactory(String connectorName) {

		}
		// TODO: check and use dynamicFilter in CarbondataPageSource

[CARBONDATA-4091] support prestosql 333 integartion with carbon #4034

[CARBONDATA-4091] support prestosql 333 integartion with carbon #4034

Conversation

ajantha-bhat commented Dec 2, 2020 • edited

Why is this PR needed?

What changes were proposed in this PR?

Does this PR introduce any user interface change?

Is any new testcase added?

CarbonDataQA2 commented Dec 2, 2020

CarbonDataQA2 commented Dec 2, 2020

CarbonDataQA2 commented Dec 8, 2020

CarbonDataQA2 commented Dec 8, 2020

CarbonDataQA2 commented Dec 8, 2020

CarbonDataQA2 commented Dec 8, 2020

CarbonDataQA2 commented Dec 9, 2020

CarbonDataQA2 commented Dec 9, 2020

CarbonDataQA2 commented Dec 18, 2020

CarbonDataQA2 commented Dec 18, 2020

CarbonDataQA2 commented Dec 18, 2020

CarbonDataQA2 commented Dec 18, 2020

CarbonDataQA2 commented Dec 18, 2020

CarbonDataQA2 commented Dec 30, 2020

CarbonDataQA2 commented Dec 30, 2020

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

presto:aj333> select * from t1; name

presto:aj333> select * from t1; name

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

CarbonDataQA2 commented Jan 8, 2021

CarbonDataQA2 commented Jan 8, 2021

CarbonDataQA2 commented Jan 13, 2021

CarbonDataQA2 commented Jan 13, 2021

ajantha-bhat commented Apr 6, 2021

CarbonDataQA2 commented Apr 6, 2021

CarbonDataQA2 commented Apr 6, 2021

ydvpankaj99 commented Apr 6, 2021

CarbonDataQA2 commented Apr 6, 2021

CarbonDataQA2 commented Apr 6, 2021

CarbonDataQA2 commented May 12, 2021

CarbonDataQA2 commented May 12, 2021

jackylk commented May 18, 2021

CarbonDataQA2 commented Aug 5, 2021

CarbonDataQA2 commented Aug 5, 2021

CarbonDataQA2 commented Aug 5, 2021

ajantha-bhat commented Aug 6, 2021

CarbonDataQA2 commented Aug 6, 2021

CarbonDataQA2 commented Aug 6, 2021

CarbonDataQA2 commented Aug 6, 2021

brijoobopanna commented Aug 10, 2021

CarbonDataQA2 commented Aug 10, 2021

CarbonDataQA2 commented Aug 10, 2021

CarbonDataQA2 commented Aug 10, 2021

akashrn5 commented Aug 10, 2021

Indhumathi27 commented Aug 11, 2021

CarbonDataQA2 commented Aug 11, 2021

CarbonDataQA2 commented Aug 11, 2021

CarbonDataQA2 commented Aug 11, 2021

ajantha-bhat commented Dec 2, 2020 •

edited

presto:aj333> select * from t1;
name

presto:aj333> select * from t1;
name