Remove dependency on Slice from core/trino-parser by wendigo · Pull Request #11856 · trinodb/trino

wendigo · 2022-04-07T20:32:37Z

Is this change a fix, improvement, new feature, refactoring, or other?

Refactoring

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java

wendigo · 2022-04-07T20:36:30Z

Let me know your thoughts @martint @electrum

findepi · 2022-04-07T20:47:04Z

core/trino-main/src/main/java/io/trino/sql/planner/LiteralInterpreter.java

AFAIK, StringLiteral contains Slice for performance reasons.
so this is not an OK change.

Can we keep byte[] instead there and only wrap as Slice in relevant places?

It used to matter when the evaluation engine relied on interpreting the AST for every row. That hasn't been the case for many years.

See previous comment: #5649 (comment)

Can we keep byte[] instead there and only wrap as Slice in relevant places?

This is not needed. The conversions from String in the analyzer/planner don't happen often enough for it to be a meaningful performance issue.

Let me benchmark Slice w/ String vs String vs byte[] with String view method

I'm benchmarking, so we will be able to compare numbers

findepi · 2022-04-07T20:49:16Z

@wendigo this is not the way to go. We're working around a problem we made ourselves to ourselves.
Crippling the code isn't a viable option, for such reason.

we either bump CLI to require Java 11
or we downgrade Slice back to Java 8
or we split trino-parser into two modules (or maybe just move part of it over to trino-main)

cc @hashhar @losipiuk @electrum

electrum

A few comments. Generally looks good. @martint should review as well

core/trino-parser/src/main/java/io/trino/sql/tree/BinaryLiteral.java

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java

core/trino-parser/src/main/java/io/trino/sql/TreePrinter.java

core/trino-parser/src/main/java/io/trino/sql/tree/CharLiteral.java

core/trino-parser/src/main/java/io/trino/sql/tree/StringLiteral.java

wendigo · 2022-04-08T09:28:01Z

@findepi: There are no significant differences in the performance after this change.

core/trino-parser/src/main/java/io/trino/sql/TreePrinter.java

core/trino-parser/src/main/java/io/trino/sql/tree/StringLiteral.java

core/trino-parser/src/main/java/io/trino/sql/tree/CharLiteral.java

findepi · 2022-04-08T09:37:32Z

There are no significant differences in the performance after this change.

That's good to know.
I am also concerned about API deterioration it's causing.
Adding third way to represent varchar values -- byte[] -- is not something I am excited about.

I would understand why we're doing this, if we had to workaround a library that we don't control, but that's not the case here.

martint · 2022-04-08T14:27:17Z

Adding third way to represent varchar values -- byte[] -- is not something I am excited about.

We should not. It should just represent them and return them as String. There's no benefit in keeping them as byte[]

wendigo · 2022-04-08T14:44:30Z

@martint ok, I'll drop this commit and push soon.

wendigo · 2022-04-08T14:48:08Z

@martint ptal now. Both Char and String literal now returns String

martint · 2022-04-08T20:53:14Z

Error:  io.trino.type.TestIpAddressOperators.testVarbinaryToIpAddressCast  Time elapsed: 0.025 s  <<< FAILURE!
java.lang.RuntimeException: Error compiling $operator$CAST([B@34a76719): Cannot cast [B to io.airlift.slice.Slice
	at io.trino.operator.scalar.FunctionAssertions.compileFilterProject(FunctionAssertions.java:829)
	at io.trino.operator.scalar.FunctionAssertions.executeProjectionWithAllInTx(FunctionAssertions.java:557)
	at io.trino.operator.scalar.FunctionAssertions.lambda$executeProjectionWithAll$9(FunctionAssertions.java:533)
	at io.trino.transaction.TransactionBuilder.execute(TransactionBuilder.java:150)
	at io.trino.operator.scalar.FunctionAssertions.executeProjectionWithAll(FunctionAssertions.java:530)
	at io.trino.operator.scalar.FunctionAssertions.selectUniqueValue(FunctionAssertions.java:340)
	at io.trino.operator.scalar.FunctionAssertions.selectSingleValue(FunctionAssertions.java:335)
	at io.trino.operator.scalar.FunctionAssertions.assertFunction(FunctionAssertions.java:298)
	at io.trino.operator.scalar.AbstractTestFunctions.assertFunction(AbstractTestFunctions.java:89)
	at io.trino.type.TestIpAddressOperators.testVarbinaryToIpAddressCast(TestIpAddressOperators.java:72)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.ClassCastException: Cannot cast [B to io.airlift.slice.Slice
	at java.base/java.lang.Class.cast(Class.java:3605)
	at java.base/java.lang.invoke.MethodHandle.bindTo(MethodHandle.java:1510)
	at java.base/java.lang.invoke.MethodHandles.constant(MethodHandles.java:3354)
	at io.trino.sql.gen.CallSiteBinder.bind(CallSiteBinder.java:42)
	at io.trino.sql.gen.RowExpressionCompiler$Visitor.visitConstant(RowExpressionCompiler.java:184)
	at io.trino.sql.gen.RowExpressionCompiler$Visitor.visitConstant(RowExpressionCompiler.java:78)
	at io.trino.sql.relational.ConstantExpression.accept(ConstantExpression.java:75)
	at io.trino.sql.gen.RowExpressionCompiler.compile(RowExpressionCompiler.java:75)
	at io.trino.sql.gen.BytecodeGeneratorContext.lambda$argumentCompiler$1(BytecodeGeneratorContext.java:106)
	at io.trino.sql.gen.BytecodeUtils.generateFullInvocation(BytecodeUtils.java:263)
	at io.trino.sql.gen.BytecodeUtils.generateFullInvocation(BytecodeUtils.java:231)
	at io.trino.sql.gen.BytecodeGeneratorContext.generateFullCall(BytecodeGeneratorContext.java:101)
	at io.trino.sql.gen.RowExpressionCompiler$Visitor.visitCall(RowExpressionCompiler.java:91)
	at io.trino.sql.gen.RowExpressionCompiler$Visitor.visitCall(RowExpressionCompiler.java:

wendigo · 2022-04-10T21:32:28Z

@martint fixed

This allows us to keep core/parser independent of Slice JDK 11 requirement.

wendigo · 2022-04-11T08:36:10Z

@martint @electrum PTAL

cla-bot bot added the cla-signed label Apr 7, 2022

wendigo requested review from electrum and martint April 7, 2022 20:32

martint reviewed Apr 7, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java Outdated Show resolved Hide resolved

findepi reviewed Apr 7, 2022

View reviewed changes

wendigo force-pushed the serafin/remove-slice-from-parser branch from 8decff4 to d12cac6 Compare April 7, 2022 20:55

wendigo requested review from findepi and martint April 7, 2022 21:17

electrum reviewed Apr 7, 2022

View reviewed changes

wendigo force-pushed the serafin/remove-slice-from-parser branch 2 times, most recently from fdaad10 to 44a66ad Compare April 8, 2022 09:27

wendigo requested a review from electrum April 8, 2022 09:28

findepi reviewed Apr 8, 2022

View reviewed changes

core/trino-parser/src/main/java/io/trino/sql/tree/StringLiteral.java Outdated Show resolved Hide resolved

core/trino-parser/src/main/java/io/trino/sql/tree/CharLiteral.java Outdated Show resolved Hide resolved

wendigo force-pushed the serafin/remove-slice-from-parser branch from 44a66ad to ec97b62 Compare April 8, 2022 09:54

wendigo requested a review from findepi April 8, 2022 10:04

wendigo force-pushed the serafin/remove-slice-from-parser branch from ec97b62 to ff4fe21 Compare April 8, 2022 14:45

martint approved these changes Apr 8, 2022

View reviewed changes

wendigo force-pushed the serafin/remove-slice-from-parser branch from ff4fe21 to eaf9de3 Compare April 10, 2022 21:27

wendigo force-pushed the serafin/remove-slice-from-parser branch 2 times, most recently from 9677aee to ade0ec3 Compare April 10, 2022 23:48

Remove Slice usage from Char, String and Binary literals

d82cc2b

This allows us to keep core/parser independent of Slice JDK 11 requirement.

wendigo added 4 commits April 11, 2022 10:30

Replace Pattern usage with CharMatcher in BinaryLiteral

6dcf440

Remove unneccesary contains check on remove

3a001f8

Simplify ExpressionAnalyzer code

f56f9d1

Remove unused TreePrinter class

08fbc39

wendigo force-pushed the serafin/remove-slice-from-parser branch from ade0ec3 to 08fbc39 Compare April 11, 2022 08:35

wendigo requested a review from martint April 11, 2022 08:36

nineinchnick mentioned this pull request Apr 11, 2022

Update airbase to version 124 #11900

Closed

martint merged commit 5fee4bc into trinodb:master Apr 12, 2022

github-actions bot added this to the 377 milestone Apr 12, 2022

wendigo deleted the serafin/remove-slice-from-parser branch April 12, 2022 21:13

mosabua mentioned this pull request Apr 13, 2022

Add Trino 377 release notes #11859

Merged

Conversation

wendigo commented Apr 7, 2022 • edited by martint Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation

Release notes

Uh oh!

Uh oh!

wendigo commented Apr 7, 2022

Uh oh!

findepi Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

wendigo Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

martint Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

martint Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

wendigo Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

wendigo Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

findepi commented Apr 7, 2022

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wendigo commented Apr 8, 2022

Uh oh!

Uh oh!

Uh oh!

findepi commented Apr 8, 2022

Uh oh!

martint commented Apr 8, 2022

Uh oh!

wendigo commented Apr 8, 2022

Uh oh!

wendigo commented Apr 8, 2022

Uh oh!

martint commented Apr 8, 2022

Uh oh!

wendigo commented Apr 10, 2022

Uh oh!

wendigo commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

wendigo commented Apr 7, 2022 •

edited by martint

Loading