Skip to content

Commit 775e856

Browse files
takezoefindepi
takezoe
authored and
findepi
committed
Accept non-orderable type in SimplePagesHashStrategy
1 parent 70464c8 commit 775e856

File tree

3 files changed

+170
-4
lines changed

3 files changed

+170
-4
lines changed

‎core/trino-main/src/main/java/io/trino/operator/SimplePagesHashStrategy.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public class SimplePagesHashStrategy
3939
{
4040
private static final int INSTANCE_SIZE = ClassLayout.parseClass(SimplePagesHashStrategy.class).instanceSize();
4141
private final List<Type> types;
42-
private final List<BlockPositionComparison> comparisonOperators;
42+
private final List<Optional<BlockPositionComparison>> comparisonOperators;
4343
private final List<Integer> outputChannels;
4444
private final List<List<Block>> channels;
4545
private final List<Integer> hashChannels;
@@ -60,7 +60,7 @@ public SimplePagesHashStrategy(
6060
{
6161
this.types = ImmutableList.copyOf(requireNonNull(types, "types is null"));
6262
this.comparisonOperators = types.stream()
63-
.map(blockTypeOperators::getComparisonOperator)
63+
.map(type -> type.isOrderable() ? Optional.of(blockTypeOperators.getComparisonOperator(type)) : Optional.<BlockPositionComparison>empty())
6464
.collect(toImmutableList());
6565
this.outputChannels = ImmutableList.copyOf(requireNonNull(outputChannels, "outputChannels is null"));
6666
this.channels = ImmutableList.copyOf(requireNonNull(channels, "channels is null"));
@@ -279,11 +279,12 @@ public boolean isPositionNull(int blockIndex, int blockPosition)
279279
public int compareSortChannelPositions(int leftBlockIndex, int leftBlockPosition, int rightBlockIndex, int rightBlockPosition)
280280
{
281281
int channel = getSortChannel();
282-
283282
Block leftBlock = channels.get(channel).get(leftBlockIndex);
284283
Block rightBlock = channels.get(channel).get(rightBlockIndex);
285284

286-
return (int) comparisonOperators.get(channel).compare(leftBlock, leftBlockPosition, rightBlock, rightBlockPosition);
285+
return (int) comparisonOperators.get(channel)
286+
.orElseThrow(() -> new IllegalArgumentException("type is not orderable"))
287+
.compare(leftBlock, leftBlockPosition, rightBlock, rightBlockPosition);
287288
}
288289

289290
@Override

‎core/trino-main/src/test/java/io/trino/operator/TestSimplePagesHashStrategy.java‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
package io.trino.operator;
15+
16+
import com.google.common.collect.ImmutableList;
17+
import io.trino.spi.Page;
18+
import io.trino.spi.block.Block;
19+
import io.trino.spi.block.IntArrayBlock;
20+
import io.trino.spi.type.MapType;
21+
import io.trino.spi.type.Type;
22+
import io.trino.spi.type.TypeOperators;
23+
import io.trino.type.BlockTypeOperators;
24+
import org.testng.annotations.Test;
25+
26+
import java.util.List;
27+
import java.util.Optional;
28+
import java.util.OptionalInt;
29+
30+
import static io.trino.spi.type.IntegerType.INTEGER;
31+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
32+
import static org.testng.Assert.assertEquals;
33+
import static org.testng.Assert.assertFalse;
34+
import static org.testng.Assert.assertTrue;
35+
36+
public class TestSimplePagesHashStrategy
37+
{
38+
@Test
39+
public void testHashRowWithIntegerType()
40+
{
41+
Block block = new IntArrayBlock(1, Optional.empty(), new int[]{1234});
42+
SimplePagesHashStrategy strategy = createSimplePagesHashStrategy(INTEGER, ImmutableList.of(block));
43+
Page page = new Page(block);
44+
45+
// This works because IntegerType is comparable.
46+
assertEquals(strategy.hashRow(0, page), -4467490526933615037L);
47+
}
48+
49+
@Test
50+
public void testHashRowWithMapType()
51+
{
52+
MapType mapType = new MapType(INTEGER, INTEGER, new TypeOperators());
53+
Block block = mapType.createBlockFromKeyValue(
54+
Optional.empty(),
55+
new int[]{0, 1},
56+
new IntArrayBlock(1, Optional.empty(), new int[]{1234}),
57+
new IntArrayBlock(1, Optional.empty(), new int[]{5678}));
58+
59+
SimplePagesHashStrategy strategy = createSimplePagesHashStrategy(mapType, ImmutableList.of(block));
60+
Page page = new Page(block);
61+
62+
// This works because MapType is comparable.
63+
assertEquals(strategy.hashRow(0, page), 451258269207618863L);
64+
}
65+
66+
@Test
67+
public void testRowEqualsRowWithIntegerType()
68+
{
69+
SimplePagesHashStrategy strategy = createSimplePagesHashStrategy(INTEGER, ImmutableList.of());
70+
71+
Page leftPage = new Page(new IntArrayBlock(1, Optional.empty(), new int[]{1234}));
72+
Page rightPage1 = new Page(new IntArrayBlock(1, Optional.empty(), new int[]{1234}));
73+
Page rightPage2 = new Page(new IntArrayBlock(1, Optional.empty(), new int[]{5678}));
74+
75+
// This works because IntegerType is comparable.
76+
assertTrue(strategy.rowEqualsRow(0, leftPage, 0, rightPage1));
77+
assertFalse(strategy.rowEqualsRow(0, leftPage, 0, rightPage2));
78+
}
79+
80+
@Test
81+
public void testRowEqualsRowWithMapType()
82+
{
83+
MapType mapType = new MapType(INTEGER, INTEGER, new TypeOperators());
84+
SimplePagesHashStrategy strategy = createSimplePagesHashStrategy(mapType, ImmutableList.of());
85+
86+
Page leftPage = new Page(mapType.createBlockFromKeyValue(
87+
Optional.empty(),
88+
new int[]{0, 1},
89+
new IntArrayBlock(1, Optional.empty(), new int[]{1234}),
90+
new IntArrayBlock(1, Optional.empty(), new int[]{5678})));
91+
92+
Page rightPage1 = new Page(mapType.createBlockFromKeyValue(
93+
Optional.empty(),
94+
new int[]{0, 1},
95+
new IntArrayBlock(1, Optional.empty(), new int[]{1234}),
96+
new IntArrayBlock(1, Optional.empty(), new int[]{5678})));
97+
98+
Page rightPage2 = new Page(mapType.createBlockFromKeyValue(
99+
Optional.empty(),
100+
new int[]{0, 1},
101+
new IntArrayBlock(1, Optional.empty(), new int[]{1234}),
102+
new IntArrayBlock(1, Optional.empty(), new int[]{1234})));
103+
104+
// This works because MapType is comparable.
105+
assertTrue(strategy.rowEqualsRow(0, leftPage, 0, rightPage1));
106+
assertFalse(strategy.rowEqualsRow(0, leftPage, 0, rightPage2));
107+
}
108+
109+
@Test
110+
public void testCompareSortChannelPositionsWithIntegerType()
111+
{
112+
Block block = new IntArrayBlock(3, Optional.empty(), new int[]{1234, 5678, 1234});
113+
SimplePagesHashStrategy strategy = createSimplePagesHashStrategy(INTEGER, ImmutableList.of(block));
114+
115+
// This works because IntegerType is orderable.
116+
assertEquals(strategy.compareSortChannelPositions(0, 0, 0, 1), -1);
117+
assertEquals(strategy.compareSortChannelPositions(0, 1, 0, 0), 1);
118+
assertEquals(strategy.compareSortChannelPositions(0, 0, 0, 2), 0);
119+
}
120+
121+
@Test
122+
public void testCompareSortChannelPositionsWithMapType()
123+
{
124+
MapType mapType = new MapType(INTEGER, INTEGER, new TypeOperators());
125+
Block block = mapType.createBlockFromKeyValue(
126+
Optional.empty(),
127+
new int[]{0, 1},
128+
new IntArrayBlock(1, Optional.empty(), new int[]{1234}),
129+
new IntArrayBlock(1, Optional.empty(), new int[]{5678}));
130+
131+
SimplePagesHashStrategy strategy = createSimplePagesHashStrategy(mapType, ImmutableList.of(block));
132+
133+
// This fails because MapType is not orderable.
134+
assertThatThrownBy(() -> strategy.compareSortChannelPositions(0, 0, 0, 0))
135+
.isInstanceOf(IllegalArgumentException.class)
136+
.hasMessageContaining("type is not orderable");
137+
}
138+
139+
private static SimplePagesHashStrategy createSimplePagesHashStrategy(Type type, List<Block> channelBlocks)
140+
{
141+
return new SimplePagesHashStrategy(
142+
ImmutableList.of(type),
143+
ImmutableList.of(),
144+
ImmutableList.of(channelBlocks),
145+
ImmutableList.of(0),
146+
OptionalInt.empty(),
147+
Optional.of(0),
148+
new BlockTypeOperators());
149+
}
150+
}

‎testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6066,6 +6066,21 @@ public void testLateMaterializationOuterJoin()
60666066
assertQuery(session, "SELECT * FROM (SELECT * FROM nation WHERE nationkey < -1) a RIGHT JOIN nation b ON a.nationkey = b.nationkey");
60676067
}
60686068

6069+
/**
6070+
* Regression test for https://github.com/trinodb/trino/pull/7723.
6071+
*/
6072+
@Test
6073+
public void testJoinWithNonOrderableType()
6074+
{
6075+
assertThat(query("SELECT b.mapped FROM (SELECT 'trino' AS name) a " +
6076+
"LEFT JOIN ( " +
6077+
" SELECT " +
6078+
" split(CAST(JSON '{\"key\": {\"name\": \"trino\"}}' AS map(varchar, map(varchar, varchar)))['key']['name'], ',') AS names, " +
6079+
" CAST(JSON '{\"key\": {\"name\": \"trino\"}}' AS map(varchar, map(varchar, varchar)))['key'] mapped " +
6080+
") b ON contains(b.names, a.name)"))
6081+
.matches("SELECT CAST(map(ARRAY['name'], ARRAY['trino']) AS map(varchar, varchar))");
6082+
}
6083+
60696084
private static ZonedDateTime zonedDateTime(String value)
60706085
{
60716086
return ZONED_DATE_TIME_FORMAT.parse(value, ZonedDateTime::from);

0 commit comments

Comments
 (0)