Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sql types SPI cleanup #24228

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+115 −40
Open

Sql types SPI cleanup #24228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ecf64a
Make SqlVarbinary map serialization consistent with other types
wendigo Nov 22, 2024
e4ba2d5
Decouple Sql types from JSON serialization
wendigo Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions core/trino-main/src/main/java/io/trino/server/protocol/JsonEncodingUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@

import java.io.IOException;
import java.math.BigDecimal;
import java.util.Base64;
import java.util.List;
import java.util.function.Consumer;

Expand Down Expand Up @@ -368,14 +367,10 @@ public void encode(JsonGenerator generator, ConnectorSession session, Block bloc
verify(keyBlock.getPositionCount() == valueBlock.getPositionCount(), "Key and value blocks have different number of positions");
generator.writeStartObject();
for (int i = 0; i < map.getSize(); i++) {
// Map keys are always serialized as strings for backward compatibility with existing clients,
// except for SqlVarbinary type which is encoded as base64-encoded string.
// Map keys are always serialized as strings for backward compatibility with existing clients.
// Map values are always properly encoded using their types.
// TODO: improve in v2 JSON format
switch (mapType.getKeyType().getObjectValue(session, keyBlock, offset + i)) {
case SqlVarbinary varbinary -> generator.writeFieldName(Base64.getEncoder().encodeToString(varbinary.getBytes()));
case Object value -> generator.writeFieldName(value.toString());
}
generator.writeFieldName(mapType.getKeyType().getObjectValue(session, keyBlock, offset + i).toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment above mentions backward compatibility. But we are changing how it works just know. Is that commment even important at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, JSON v1 format always serializes key to a String, even if it is a map or list.

valueEncoder.encode(generator, session, valueBlock, offset + i);
}
generator.writeEndObject();
Expand Down
8 changes: 4 additions & 4 deletions ...in/src/test/java/io/trino/operator/aggregation/TestQuantileDigestAggregationFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,11 +363,11 @@ private void assertPercentileWithinError(String type, SqlVarbinary binary, doubl
// Check that the chosen quantile is within the upper and lower bound of the error
assertThat(assertions.expression(
format("value_at_quantile(CAST(a AS qdigest(%s)), %s) >= %s", type, percentile, lowerBound))
.binding("a", "X'%s'".formatted(binary.toString().replaceAll("\\s+", " "))))
.binding("a", "X'%s'".formatted(binary.toHexString().replaceAll("\\s+", " "))))
.isEqualTo(true);
assertThat(assertions.expression(
format("value_at_quantile(CAST(a AS qdigest(%s)), %s) <= %s", type, percentile, upperBound))
.binding("a", "X'%s'".formatted(binary.toString().replaceAll("\\s+", " "))))
.binding("a", "X'%s'".formatted(binary.toHexString().replaceAll("\\s+", " "))))
.isEqualTo(true);
}

Expand All @@ -384,7 +384,7 @@ private void assertPercentilesWithinError(String type, SqlVarbinary binary, doub
type,
ARRAY_JOINER.join(boxedPercentiles),
ARRAY_JOINER.join(lowerBounds)))
.binding("a", "X'%s'".formatted(binary.toString().replaceAll("\\s+", " "))))
.binding("a", "X'%s'".formatted(binary.toHexString().replaceAll("\\s+", " "))))
.hasType(new ArrayType(BOOLEAN))
.isEqualTo(Collections.nCopies(percentiles.length, true));

Expand All @@ -395,7 +395,7 @@ private void assertPercentilesWithinError(String type, SqlVarbinary binary, doub
type,
ARRAY_JOINER.join(boxedPercentiles),
ARRAY_JOINER.join(upperBounds)))
.binding("a", "X'%s'".formatted(binary.toString().replaceAll("\\s+", " "))))
.binding("a", "X'%s'".formatted(binary.toHexString().replaceAll("\\s+", " "))))
.hasType(new ArrayType(BOOLEAN))
.isEqualTo(Collections.nCopies(percentiles.length, true));
}
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/test/java/io/trino/operator/aggregation/TestQuantileDigestFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,6 @@ private static void addAll(QuantileDigest digest, long... values)

private static String toHexString(QuantileDigest qdigest)
{
return new SqlVarbinary(qdigest.serialize().getBytes()).toString().replaceAll("\\s+", " ");
return new SqlVarbinary(qdigest.serialize().getBytes()).toHexString().replaceAll("\\s+", " ");
}
}
56 changes: 56 additions & 0 deletions core/trino-spi/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,62 @@
<old>method io.trino.spi.connector.RecordCursor io.trino.spi.connector.SystemTable::cursor(io.trino.spi.connector.ConnectorTransactionHandle, io.trino.spi.connector.ConnectorSession, io.trino.spi.predicate.TupleDomain&lt;java.lang.Integer&gt;, java.util.Set&lt;java.lang.Integer&gt;)</old>
<new>method io.trino.spi.connector.RecordCursor io.trino.spi.connector.SystemTable::cursor(io.trino.spi.connector.ConnectorTransactionHandle, io.trino.spi.connector.ConnectorSession, io.trino.spi.predicate.TupleDomain&lt;java.lang.Integer&gt;, java.util.Set&lt;java.lang.Integer&gt;, io.trino.spi.connector.ConnectorSplit)</new>
</item>
<item>
<ignore>true</ignore>
<code>java.annotation.removed</code>
<old>method byte[] io.trino.spi.type.SqlVarbinary::getBytes()</old>
<new>method byte[] io.trino.spi.type.SqlVarbinary::getBytes()</new>
<annotation>@com.fasterxml.jackson.annotation.JsonValue</annotation>
<justification>On-the-wire representation shouldn't rely on the Jackson format</justification>
</item>
<item>
<ignore>true</ignore>
<code>java.annotation.removed</code>
<old>method java.lang.String io.trino.spi.type.SqlDate::toString()</old>
<new>method java.lang.String io.trino.spi.type.SqlDate::toString()</new>
<annotation>@com.fasterxml.jackson.annotation.JsonValue</annotation>
<justification>On-the-wire representation shouldn't rely on the Jackson format</justification>
</item>
<item>
<ignore>true</ignore>
<code>java.annotation.removed</code>
<old>method java.lang.String io.trino.spi.type.SqlDecimal::toString()</old>
<new>method java.lang.String io.trino.spi.type.SqlDecimal::toString()</new>
<annotation>@com.fasterxml.jackson.annotation.JsonValue</annotation>
<justification>On-the-wire representation shouldn't rely on the Jackson format</justification>
</item>
<item>
<ignore>true</ignore>
<code>java.annotation.removed</code>
<old>method java.lang.String io.trino.spi.type.SqlTime::toString()</old>
<new>method java.lang.String io.trino.spi.type.SqlTime::toString()</new>
<annotation>@com.fasterxml.jackson.annotation.JsonValue</annotation>
<justification>On-the-wire representation shouldn't rely on the Jackson format</justification>
</item>
<item>
<ignore>true</ignore>
<code>java.annotation.removed</code>
<old>method java.lang.String io.trino.spi.type.SqlTimeWithTimeZone::toString()</old>
<new>method java.lang.String io.trino.spi.type.SqlTimeWithTimeZone::toString()</new>
<annotation>@com.fasterxml.jackson.annotation.JsonValue</annotation>
<justification>On-the-wire representation shouldn't rely on the Jackson format</justification>
</item>
<item>
<ignore>true</ignore>
<code>java.annotation.removed</code>
<old>method java.lang.String io.trino.spi.type.SqlTimestamp::toString()</old>
<new>method java.lang.String io.trino.spi.type.SqlTimestamp::toString()</new>
<annotation>@com.fasterxml.jackson.annotation.JsonValue</annotation>
<justification>On-the-wire representation shouldn't rely on the Jackson format</justification>
</item>
<item>
<ignore>true</ignore>
<code>java.annotation.removed</code>
<old>method java.lang.String io.trino.spi.type.SqlTimestampWithTimeZone::toString()</old>
<new>method java.lang.String io.trino.spi.type.SqlTimestampWithTimeZone::toString()</new>
<annotation>@com.fasterxml.jackson.annotation.JsonValue</annotation>
<justification>On-the-wire representation shouldn't rely on the Jackson format</justification>
</item>
</differences>
</revapi.differences>
</analysisConfiguration>
Expand Down
3 changes: 0 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/SqlDate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
*/
package io.trino.spi.type;

import com.fasterxml.jackson.annotation.JsonValue;

import java.time.LocalDate;

public final class SqlDate
Expand Down Expand Up @@ -51,7 +49,6 @@ public boolean equals(Object obj)
return days == other.days;
}

@JsonValue
@Override
public String toString()
{
Expand Down
3 changes: 0 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/SqlDecimal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
*/
package io.trino.spi.type;

import com.fasterxml.jackson.annotation.JsonValue;

import java.math.BigDecimal;
import java.math.BigInteger;
import java.math.MathContext;
Expand Down Expand Up @@ -81,7 +79,6 @@ public int hashCode()
return Objects.hash(unscaledValue, precision, scale);
}

@JsonValue
@Override
public String toString()
{
Expand Down
3 changes: 0 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/SqlTime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
*/
package io.trino.spi.type;

import com.fasterxml.jackson.annotation.JsonValue;

import java.util.Objects;

import static io.trino.spi.type.TimeType.MAX_PRECISION;
Expand Down Expand Up @@ -85,7 +83,6 @@ public int hashCode()
return Objects.hash(precision, picos);
}

@JsonValue
@Override
public String toString()
{
Expand Down
3 changes: 0 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/SqlTimeWithTimeZone.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
*/
package io.trino.spi.type;

import com.fasterxml.jackson.annotation.JsonValue;

import java.util.Objects;

import static io.trino.spi.type.TimeType.MAX_PRECISION;
Expand Down Expand Up @@ -99,7 +97,6 @@ public int hashCode()
return Objects.hash(precision, picos, offsetMinutes);
}

@JsonValue
@Override
public String toString()
{
Expand Down
3 changes: 0 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/SqlTimestamp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
*/
package io.trino.spi.type;

import com.fasterxml.jackson.annotation.JsonValue;

import java.time.LocalDateTime;
import java.time.ZoneOffset;
import java.util.Objects;
Expand Down Expand Up @@ -149,7 +147,6 @@ public int hashCode()
return Objects.hash(epochMicros, picosOfMicros, precision);
}

@JsonValue
@Override
public String toString()
{
Expand Down
3 changes: 0 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/SqlTimestampWithTimeZone.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
*/
package io.trino.spi.type;

import com.fasterxml.jackson.annotation.JsonValue;

import java.time.Instant;
import java.time.ZoneId;
import java.time.ZonedDateTime;
Expand Down Expand Up @@ -132,7 +130,6 @@ public SqlTimestampWithTimeZone roundTo(int precision)
return newInstanceWithRounding(precision, epochMillis, picosOfMilli, timeZoneKey);
}

@JsonValue
@Override
public String toString()
{
Expand Down
9 changes: 6 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/SqlVarbinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@
*/
package io.trino.spi.type;

import com.fasterxml.jackson.annotation.JsonValue;

import java.util.Arrays;
import java.util.Base64;
import java.util.HexFormat;

import static java.lang.Math.min;
Expand All @@ -42,7 +41,6 @@ public int compareTo(SqlVarbinary obj)
return Arrays.compare(bytes, obj.bytes);
}

@JsonValue
public byte[] getBytes()
{
return bytes;
Expand All @@ -69,6 +67,11 @@ public boolean equals(Object obj)

@Override
public String toString()
{
return Base64.getEncoder().encodeToString(bytes);
}

public String toHexString()
{
if (bytes.length == 0) {
return "";
Expand Down
11 changes: 7 additions & 4 deletions core/trino-spi/src/test/java/io/trino/spi/type/TestSqlVarbinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,27 @@

import org.junit.jupiter.api.Test;

import java.util.Base64;

import static java.lang.String.format;
import static org.assertj.core.api.Assertions.assertThat;

public class TestSqlVarbinary
{
@Test
public void testToString()
public void testToHexString()
{
for (int lines = 0; lines < 5; lines++) {
for (int lastLineBytes = 0; lastLineBytes < 32; lastLineBytes++) {
byte[] bytes = createBytes(lines, lastLineBytes);
String expected = simpleToString(bytes);
assertThat(expected).isEqualTo(new SqlVarbinary(bytes).toString());
String expectedHex = simpleToHex(bytes);
assertThat(expectedHex).isEqualTo(new SqlVarbinary(bytes).toHexString());
assertThat(Base64.getEncoder().encodeToString(bytes)).isEqualTo(new SqlVarbinary(bytes).toString());
}
}
}

private static String simpleToString(byte[] bytes)
private static String simpleToHex(byte[] bytes)
{
StringBuilder builder = new StringBuilder();

Expand Down
36 changes: 34 additions & 2 deletions lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/util/JsonTypeUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,26 @@
import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import io.airlift.json.ObjectMapperProvider;
import io.airlift.slice.DynamicSliceOutput;
import io.airlift.slice.Slice;
import io.airlift.slice.SliceOutput;
import io.airlift.slice.Slices;
import io.trino.spi.TrinoException;
import io.trino.spi.type.SqlDate;
import io.trino.spi.type.SqlDecimal;
import io.trino.spi.type.SqlTime;
import io.trino.spi.type.SqlTimeWithTimeZone;
import io.trino.spi.type.SqlTimestamp;
import io.trino.spi.type.SqlTimestampWithTimeZone;
import io.trino.spi.type.SqlVarbinary;

import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.util.List;

import static com.fasterxml.jackson.core.JsonFactory.Feature.CANONICALIZE_FIELD_NAMES;
import static com.fasterxml.jackson.databind.SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS;
Expand Down Expand Up @@ -63,10 +73,32 @@ public static Slice jsonParse(Slice slice)
}
}

public static Slice toJsonValue(Object value)
public static Slice toJsonValue(List<?> values)
throws IOException
{
return Slices.wrappedBuffer(SORTED_MAPPER.writeValueAsBytes(value));
if (values == null) {
return Slices.utf8Slice("[]");
}

ImmutableList.Builder<String> json = ImmutableList.builder();
for (Object value : values) {
if (value == null) {
json.add("null");
continue;
}
json.add(switch (value) {
case SqlDate _,
SqlTime _,
SqlVarbinary _,
SqlTimeWithTimeZone _,
SqlDecimal _,
SqlTimestamp _,
SqlTimestampWithTimeZone _ -> SORTED_MAPPER.writeValueAsString(value.toString());
default -> SORTED_MAPPER.writeValueAsString(value);
});
}

return Slices.utf8Slice("[" + Joiner.on(",").join(json.build()) + "]");
}

private static JsonParser createJsonParser(JsonFactory factory, Slice json)
Expand Down
6 changes: 5 additions & 1 deletion plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,8 +1474,12 @@ private static SliceReadFunction arrayAsJsonReadFunction(ConnectorSession sessio
type.writeObject(builder, block);
Object value = type.getObjectValue(session, builder.build(), 0);

if (!(value instanceof List<?> list)) {
throw new TrinoException(JDBC_ERROR, "Conversion to JSON failed for " + type.getDisplayName());
}

try {
return toJsonValue(value);
return toJsonValue(list);
}
catch (IOException e) {
throw new TrinoException(JDBC_ERROR, "Conversion to JSON failed for " + type.getDisplayName(), e);
Expand Down
Loading