Add a config flag `tpcds.use-varchar-type` in Presto - Java TPC-DS Connector #24406

pdabre12 · 2025-01-21T17:11:04Z

Description

Add a config flag tpcds.use-varchar-type in Presto - Java TPC-DS Connector

Motivation and Context

Resolves: #24362

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

TPC-DS Connector Changes
* Add config property ``tpcds.use-varchar-type`` to allow toggling of char columns to varchar columns.

steveburnett

Thanks for the doc! Unfortunately there's a problem in RST formatting that simple tables - the way the table was initially formatted - "the first column cells cannot contain multiple lines".

See https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#tables for details.

As a result, adding a second row causes the table to build looking like this:

In a local doc build I recoded the table as a grid table (formatting in the same link above) and a new doc build looked like this:

If it helps, here's a screenshot of the grid table format I used to build the second screenshot in Visual Studio:

If you redo the table in grid table formatting like I did, it should be fine.

pdabre12 · 2025-01-21T19:32:10Z

Thanks @steveburnett for the detailed explanation.
I changed the table to be similar to other connectors.rst files eg: tpch.rst. It renders correctly locally for me.
Please take a look.

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good. Thanks!

pdabre12 · 2025-01-22T19:00:41Z

@aditi-pandit @majetideepak Please take a look, thanks.

aditi-pandit

@pdabre12 : Thanks for this code. Have a question about testing... Would it be possible to run these tests https://github.com/prestodb/presto/blob/master/presto-tpcds/src/test/java/com/facebook/presto/tpcds/TestTpcds.java with the flag turned on and off to validate the correctness of this work.

pdabre12 · 2025-01-31T22:13:14Z

@aditi-pandit Added the test cases, PTAL.
Thanks.

steveburnett · 2025-02-03T14:58:44Z

New release note guidelines as of last week: PR #24354 automatically adds links to this PR to the release notes. Please remove the manual PR link in the following format from the release note entries for this PR.

:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

steveburnett

LGTM! (docs)

Pull updated branch, new doc build, looks good.

steveburnett · 2025-02-03T17:36:49Z

The updated release note entry looks good, thank you!

aditi-pandit · 2025-02-11T01:11:00Z

presto-tpcds/src/main/java/com/facebook/presto/tpcds/TpcdsConnectorFactory.java

@@ -64,6 +64,8 @@ public ConnectorHandleResolver getHandleResolver()
    public Connector create(String catalogName, Map<String, String> config, ConnectorContext context)
    {
        int splitsPerNode = getSplitsPerNode(config);
+        // cast char columns to varchar only in a native cluster
+        boolean useVarcharType = context.getConnectorSystemConfig().isNativeExecution() && useVarcharType(config);


There is a different way to think about this.

The Java TPCDS connector could work with either char or varchar. But Native requires useVarcharType. So the code should error if isNativeExecution but useVarcharType is not set. We want to support varcharType with Java engine for compatibility tests with native engine.

Would be good to change useVarcharType(config) with the validation for native execution.

Made the change, thanks.

aditi-pandit · 2025-02-11T01:13:35Z

presto-tpcds/src/main/java/com/facebook/presto/tpcds/TpcdsRecordSet.java

+            // The TPCDS config tpcds.use-varchar-type is set to true only for native execution, since char type is
+            // currently unsupported in Presto native. This function is only used by the Presto java TPCDS connector
+            // so the argument useVarcharType is false here.
+            columnTypes.add(getPrestoType(column.getType(), false));


This should use the tpcds.use-varchar-type config. Java should support both options.

Co-authored-by: Pramod Satya <[email protected]>

aditi-pandit · 2025-02-13T21:26:56Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergQueryRunner.java

@@ -161,10 +161,11 @@ public static DistributedQueryRunner createIcebergQueryRunner(
            OptionalInt nodeCount,
            Optional<BiFunction<Integer, URI, Process>> externalWorkerLauncher,
            Optional<Path> dataDirectory,
-            boolean addStorageFormatToPath)
+            boolean addStorageFormatToPath,
+            Map<String, String> tpcdsProperties)


@pdabre12 : Do you want to do a separate input parameter ? Another option could be to add these in extraConnectorProperties and check them when initializing the plugin.. See how they did for Iceberg catalog : https://github.com/prestodb/presto/blob/master/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergQueryRunner.java#L211

I could do that, but would it make sense to have TPCDS connector specific properties in extraConnectorProperties? It seems that is used to pass down iceberg specific properties.

Well, guess we would need to filter properties for individual connectors if we mixed them. Alright, lets keep tpcds separated for now. Anyways we might remove this property as well.

aditi-pandit

@pdabre12 : Since you add this config, we could remove all the varchar casting in the table creations here https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeTpcdsQueries.java#L85.

Can you try that ?

aditi-pandit · 2025-02-13T21:56:04Z

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java

Since the java runners in this file are used to validate against native workers, should we be setting tpcds.use-varchar-type for them as well ?

prestodb-ci added the from:IBM PR from IBM label Jan 21, 2025

prestodb-ci requested review from a team, czentgr and psnv03 and removed request for a team January 21, 2025 17:11

steveburnett requested changes Jan 21, 2025

View reviewed changes

pdabre12 force-pushed the tpcds-use-varchar-config branch 2 times, most recently from ef3e428 to f2ef87b Compare January 21, 2025 19:27

pdabre12 requested a review from steveburnett January 21, 2025 19:33

steveburnett previously approved these changes Jan 21, 2025

View reviewed changes

pdabre12 marked this pull request as ready for review January 21, 2025 19:58

pdabre12 requested review from elharo and a team as code owners January 21, 2025 19:58

pdabre12 requested a review from presto-oss January 21, 2025 19:58

aditi-pandit reviewed Jan 23, 2025

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 7b7e54c January 31, 2025 22:10

pdabre12 force-pushed the tpcds-use-varchar-config branch from f2ef87b to 7b7e54c Compare January 31, 2025 22:10

pdabre12 requested a review from aditi-pandit January 31, 2025 23:31

pdabre12 requested a review from steveburnett February 3, 2025 17:15

steveburnett previously approved these changes Feb 3, 2025

View reviewed changes

aditi-pandit reviewed Feb 11, 2025

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 1e7a0d6 February 12, 2025 19:59

pdabre12 force-pushed the tpcds-use-varchar-config branch from 7b7e54c to 1e7a0d6 Compare February 12, 2025 19:59

pdabre12 requested a review from aditi-pandit February 12, 2025 20:02

pdabre12 force-pushed the tpcds-use-varchar-config branch from 1e7a0d6 to 1cf3d33 Compare February 12, 2025 22:08

pdabre12 requested a review from a team as a code owner February 12, 2025 22:08

Add config property 'tpcds.use-varchar-type'

5c41a45

Co-authored-by: Pramod Satya <[email protected]>

pdabre12 force-pushed the tpcds-use-varchar-config branch from 1cf3d33 to 5c41a45 Compare February 13, 2025 19:55

pdabre12 requested review from hantangwangd and ZacBlanco as code owners February 13, 2025 19:55

aditi-pandit reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a config flag `tpcds.use-varchar-type` in Presto - Java TPC-DS Connector #24406

Add a config flag `tpcds.use-varchar-type` in Presto - Java TPC-DS Connector #24406

pdabre12 commented Jan 21, 2025 •

edited

Loading

steveburnett left a comment

pdabre12 commented Jan 21, 2025

steveburnett left a comment

pdabre12 commented Jan 22, 2025

aditi-pandit left a comment

pdabre12 commented Jan 31, 2025

steveburnett commented Feb 3, 2025

steveburnett left a comment

steveburnett commented Feb 3, 2025

aditi-pandit Feb 11, 2025

pdabre12 Feb 12, 2025

aditi-pandit Feb 11, 2025

aditi-pandit Feb 13, 2025

pdabre12 Feb 13, 2025

aditi-pandit Feb 13, 2025

aditi-pandit left a comment

aditi-pandit Feb 13, 2025

Add a config flag tpcds.use-varchar-type in Presto - Java TPC-DS Connector #24406

Are you sure you want to change the base?

Add a config flag tpcds.use-varchar-type in Presto - Java TPC-DS Connector #24406

Conversation

pdabre12 commented Jan 21, 2025 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

steveburnett left a comment

Choose a reason for hiding this comment

pdabre12 commented Jan 21, 2025

steveburnett left a comment

Choose a reason for hiding this comment

pdabre12 commented Jan 22, 2025

aditi-pandit left a comment

Choose a reason for hiding this comment

pdabre12 commented Jan 31, 2025

steveburnett commented Feb 3, 2025

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett commented Feb 3, 2025

aditi-pandit Feb 11, 2025

Choose a reason for hiding this comment

pdabre12 Feb 12, 2025

Choose a reason for hiding this comment

aditi-pandit Feb 11, 2025

Choose a reason for hiding this comment

aditi-pandit Feb 13, 2025

Choose a reason for hiding this comment

pdabre12 Feb 13, 2025

Choose a reason for hiding this comment

aditi-pandit Feb 13, 2025

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit Feb 13, 2025

Choose a reason for hiding this comment

Add a config flag `tpcds.use-varchar-type` in Presto - Java TPC-DS Connector #24406

Add a config flag `tpcds.use-varchar-type` in Presto - Java TPC-DS Connector #24406

pdabre12 commented Jan 21, 2025 •

edited

Loading