Support integral cast projection pushdown in redshift #22951

krvikash · 2024-08-06T09:08:50Z

Description

This PR adds support for cast projection pushdown for the Oracle connector. This uses the framework introduced in #22203 for cast projection pushdown.
Currently, this PR only adds support for the SMALLINT, INT, BIGINT type cast pushdown.

The below Source type can be cast pushdown to SMALLINT, INT, BIGINT type

Boolean
SmallInt
Integer
Bigint
Decimal

Other source types not listed above are not supported for cast pushdown.

Release notes

(X) Release notes are required, with the following suggested text:

# Redshift
* Improve performance for queries casting columns to smallint or to integer or to bigint. ({issue}`22951`)

ssheikin · 2024-08-12T10:42:13Z

plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftClient.java

+            case Types.TINYINT:
+                // TODO what will be the impact here after enabling this
+                // -- Redshift doesn't support tinyint
+                return Optional.of(tinyintColumnMapping());


IIUC this will be just a dead code.

If Redshift doesn't support tinyint - how would the cast would look like ?

ssheikin · 2024-08-12T10:51:53Z

plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RewriteCast.java

+        if (isDecimalType(sourceType) && isIntegralType(targetType)) {
+            return "CAST(ROUND(%s) AS %s)".formatted(expression, castType);
+        }


I have some doubts if the trino behaviour is a correct one.
Do you know the internals of the trino behaviour?
Maybe at least add a comment here to match trino behaviour? Not sure if it's helpful (kind of obvious if comment exists, and not that obvious if not)

In Trino cast from double to any other type we use DoubleOperators and here is one example where casting from double to long roundup the value.

trino/core/trino-main/src/main/java/io/trino/type/DoubleOperators.java

Line 165 in 5f191af

return DoubleMath.roundToLong(value, HALF_UP);

Anyway, we will not support Float/Double value cast to other types, since for NaN and Infinity values we are getting behavior deference between with and without pushdown.

krvikash · 2024-08-13T08:30:24Z

Hi @ebyhr | @Praveen2112 Could you please run this PR with secrets?

ebyhr · 2024-08-13T08:35:54Z

@krvikash Sure, but did you miss adding TestRedshiftCastPushdown to cloud-tests profile in Redshift pom.xml?

krvikash · 2024-08-13T13:12:27Z

Thanks @ebyhr for reminding me. Added test in cloud-tests profile

ebyhr · 2024-08-13T13:16:38Z

/test-with-secrets sha=4c2701c4942bf52adc08044fef48887489eb020b

github-actions · 2024-08-13T14:08:54Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/10370817481

github-actions · 2024-09-03T17:02:54Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

krvikash · 2024-09-04T08:36:43Z

Once #22728 is merged, I will continue working on this PR.

krvikash · 2024-09-26T10:37:40Z

(rebased with master)

krvikash · 2024-09-26T11:50:52Z

(fix error prone check)

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftConnectorTest.java

krvikash · 2024-10-01T06:49:25Z

Hi @ebyhr Could you please run this PR against secrets?

ebyhr · 2024-10-01T06:55:36Z

/test-with-secrets sha=92d956338dfa3d357483d8297fe06a4b2e6d94b5

krvikash · 2024-10-07T12:58:59Z

(uncommented the test)

krvikash · 2024-10-08T10:19:03Z

The PR is ready for another round of review.

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java

krvikash · 2024-10-10T20:20:21Z

Thanks @mayankvadariya for the review.

Refactored the test cases.

krvikash · 2024-10-11T12:41:18Z

(rebased with master)

plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RewriteCast.java

Praveen2112 · 2024-10-15T06:08:41Z

plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RewriteCast.java

+        return "CAST(%s AS %s)".formatted(expression, castType);
+    }
+
+    private boolean isIntegralType(Type type)


Isn't this check redundant ?

It's redundant as of now since with this PR we are only supporting integral type cast. This is to ensure that in future if we are supporting other types (like varchar) then we should not fall under CAST(ROUND(%s) AS %s).

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java

Praveen2112 · 2024-10-15T06:30:07Z

plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftClient.java

+            case Types.TINYINT:
+                // -- Redshift doesn't support tinyint, but requires
+                // tinyint data type mapping for CAST pushdown
+                return Optional.of(tinyintColumnMapping());


We map that column as smallint and read them as tinyint so things would fail at the time of reading ? What if the expression is hidden within the query like some sort of a filter and this column doesn't need to be read by the underlying system.

Praveen2112 · 2024-10-15T08:18:13Z

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java

+    public void setupTable()
+    {
+        left = closeAfterClass(CastDataTypeTestTable.create(3)
+                .addColumn("id", "int", asList(11, 12, 13))


As a follow-up I think it would be nice if we could capture the cast equivalent data so it would be easy to know on how cast would end up

krvikash · 2024-10-15T09:25:35Z

Thanks @Praveen2112 for the review. ACs.

Praveen2112 · 2024-10-15T09:40:55Z

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java

+                        new InvalidCastTestCase("c_decimal_16", "bigint", "Cannot cast '9223372036854775808.49' to BIGINT", "(?s).*Value out of range for 8 bytes(?s).*"),
+
+                        // No pushdown for double datatype to integral types
+                        new InvalidCastTestCase("c_infinity_1", "tinyint", "Out of range for tinyint: Infinity"),


How does Trino handles it out without this pushdown enabled ?

It uses DoubleOperators#castToTinyint for casting

trino/core/trino-main/src/main/java/io/trino/type/DoubleOperators.java

Lines 147 to 158 in 3c20a5c

public static long castToTinyint(@SqlType(StandardTypes.DOUBLE) double value)

{

if (Double.isNaN(value)) {

throw new TrinoException(INVALID_CAST_ARGUMENT, "Cannot cast double NaN to tinyint");

}

try {

return SignedBytes.checkedCast((long) MathFunctions.round(value));

}

catch (IllegalArgumentException e) {

throw new TrinoException(NUMERIC_VALUE_OUT_OF_RANGE, "Out of range for tinyint: " + value, e);

}

}

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java

Praveen2112 · 2024-10-15T09:43:17Z

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java

+    protected List<InvalidCastTestCase> invalidCast()
+    {
+        return ImmutableList.of(
+                new InvalidCastTestCase("c_varchar_decimal", "integer"),


Can we capture the error message on Trino and redshift side ?

These data types in invalidCast does not get pushdown, So can't capture the error message here. Trino Error message (.*)Cannot cast (.*) to (.*) is already mentioned inside InvalidCastTestCase. Should be explicitly mention error message here?

Praveen2112 · 2024-10-15T09:45:32Z

@krvikash Lets update the PR description as well

Praveen2112

% comment

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java

krvikash · 2024-10-15T13:12:54Z

@Praveen2112 can you please run this PR against secrets?

Praveen2112 · 2024-10-15T15:02:54Z

/test-with-secrets sha=b5c69fafda2e99316c2a501e786d08978b6bad9b

github-actions · 2024-10-15T15:04:02Z

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/11348864763

krvikash · 2024-10-15T15:51:55Z

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/11348864763

Passed

cla-bot bot added the cla-signed label Aug 6, 2024

krvikash requested review from ssheikin, Praveen2112, SemionPar, vlad-lyutenko and marcinsbd August 6, 2024 09:12

ssheikin reviewed Aug 12, 2024

View reviewed changes

krvikash force-pushed the krvikash/redshift-cast-pushdown branch 3 times, most recently from 55d618d to 1761934 Compare August 13, 2024 08:29

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from 1761934 to 4c2701c Compare August 13, 2024 13:11

github-actions bot added the stale label Sep 3, 2024

krvikash self-assigned this Sep 4, 2024

github-actions bot removed the stale label Sep 4, 2024

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from 4c2701c to ef7405b Compare September 26, 2024 10:37

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from ef7405b to 1d0542e Compare September 26, 2024 11:50

mayankvadariya reviewed Sep 27, 2024

View reviewed changes

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftConnectorTest.java Outdated Show resolved Hide resolved

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from 1d0542e to 92d9563 Compare October 1, 2024 06:46

krvikash changed the title ~~Support cast projection pushdown in redshift~~ Support integral cast projection pushdown in redshift Oct 7, 2024

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from 9aebd47 to f6cb700 Compare October 7, 2024 12:58

mayankvadariya reviewed Oct 9, 2024

View reviewed changes

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java Outdated Show resolved Hide resolved

krvikash mentioned this pull request Oct 10, 2024

Cast pushdown test case refactoring #23737

Merged

mayankvadariya approved these changes Oct 10, 2024

View reviewed changes

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java Outdated Show resolved Hide resolved

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from f6cb700 to 186e231 Compare October 10, 2024 20:19

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from 186e231 to a80b450 Compare October 11, 2024 12:40

Praveen2112 reviewed Oct 15, 2024

View reviewed changes

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from a80b450 to d93fc2d Compare October 15, 2024 09:23

Praveen2112 reviewed Oct 15, 2024

View reviewed changes

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from d93fc2d to f37d54c Compare October 15, 2024 10:17

Praveen2112 approved these changes Oct 15, 2024

View reviewed changes

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java Outdated Show resolved Hide resolved

Support integral cast projection pushdown in redshift

b5c69fa

krvikash force-pushed the krvikash/redshift-cast-pushdown branch from f37d54c to b5c69fa Compare October 15, 2024 13:12

marcinsbd approved these changes Oct 15, 2024

View reviewed changes

Praveen2112 merged commit e4607a3 into trinodb:master Oct 16, 2024
61 checks passed

github-actions bot added this to the 462 milestone Oct 16, 2024

krvikash deleted the krvikash/redshift-cast-pushdown branch October 16, 2024 05:58

mosabua mentioned this pull request Oct 16, 2024

Add Trino 462 release notes #23745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support integral cast projection pushdown in redshift #22951

Support integral cast projection pushdown in redshift #22951

krvikash commented Aug 6, 2024 •

edited

Loading

ssheikin Aug 12, 2024

Praveen2112 Sep 26, 2024

ssheikin Aug 12, 2024

krvikash Aug 13, 2024

krvikash commented Aug 13, 2024

ebyhr commented Aug 13, 2024

krvikash commented Aug 13, 2024

ebyhr commented Aug 13, 2024

github-actions bot commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024

krvikash commented Sep 4, 2024

krvikash commented Sep 26, 2024

krvikash commented Sep 26, 2024

krvikash commented Oct 1, 2024

ebyhr commented Oct 1, 2024

krvikash commented Oct 7, 2024

krvikash commented Oct 8, 2024

krvikash commented Oct 10, 2024 •

edited

Loading

krvikash commented Oct 11, 2024

Praveen2112 Oct 15, 2024

krvikash Oct 15, 2024

Praveen2112 Oct 15, 2024

Praveen2112 Oct 15, 2024

krvikash commented Oct 15, 2024

Praveen2112 Oct 15, 2024

krvikash Oct 15, 2024

Praveen2112 Oct 15, 2024

krvikash Oct 15, 2024

Praveen2112 commented Oct 15, 2024

Praveen2112 left a comment

krvikash commented Oct 15, 2024

Praveen2112 commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

krvikash commented Oct 15, 2024

	public static long castToTinyint(@SqlType(StandardTypes.DOUBLE) double value)
	{
	if (Double.isNaN(value)) {
	throw new TrinoException(INVALID_CAST_ARGUMENT, "Cannot cast double NaN to tinyint");
	}
	try {
	return SignedBytes.checkedCast((long) MathFunctions.round(value));
	}
	catch (IllegalArgumentException e) {
	throw new TrinoException(NUMERIC_VALUE_OUT_OF_RANGE, "Out of range for tinyint: " + value, e);
	}
	}

Support integral cast projection pushdown in redshift #22951

Support integral cast projection pushdown in redshift #22951

Conversation

krvikash commented Aug 6, 2024 • edited Loading

Description

Release notes

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

krvikash commented Aug 13, 2024

ebyhr commented Aug 13, 2024

krvikash commented Aug 13, 2024

ebyhr commented Aug 13, 2024

github-actions bot commented Aug 13, 2024 • edited Loading

github-actions bot commented Sep 3, 2024

krvikash commented Sep 4, 2024

krvikash commented Sep 26, 2024

krvikash commented Sep 26, 2024

krvikash commented Oct 1, 2024

ebyhr commented Oct 1, 2024

krvikash commented Oct 7, 2024

krvikash commented Oct 8, 2024

krvikash commented Oct 10, 2024 • edited Loading

krvikash commented Oct 11, 2024

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

krvikash commented Oct 15, 2024

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

Praveen2112 commented Oct 15, 2024

Praveen2112 left a comment

Choose a reason for hiding this comment

krvikash commented Oct 15, 2024

Praveen2112 commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

krvikash commented Oct 15, 2024

krvikash commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

krvikash commented Oct 10, 2024 •

edited

Loading