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

Deprecate implicit String-to-UTF8-bytes ticket methods #5405

Merged
merged 3 commits into from
May 14, 2024
Merged

Deprecate implicit String-to-UTF8-bytes ticket methods #5405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ff0783
Deprecate implicit String-to-UTF8-bytes ticket methods
devinrsmith Apr 24, 2024
0c8c14e
cleanup doc
devinrsmith Apr 24, 2024
1f7ca4a
Merge remote-tracking branch 'upstream/main' into ticket-table-deprec…
devinrsmith May 10, 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: 6 additions & 3 deletions engine/api/src/main/java/io/deephaven/engine/table/TableFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.deephaven.qst.table.TicketTable;
import io.deephaven.qst.table.TimeTable;

import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.time.Instant;
import java.util.Arrays;
Expand Down Expand Up @@ -221,14 +222,16 @@ public static Table merge(Table[] tables) {
}

/**
* Equivalent to {@code of(TicketTable.of(ticket))}.
* Equivalent to {@code of(TicketTable.of(ticket.getBytes(StandardCharsets.UTF_8)))}.
*
* @param ticket the ticket string
* @return the ticket table
* @see TicketTable#of(String)
* @see TicketTable#of(byte[])
* @deprecated prefer {@link #ticket(byte[])}
*/
@Deprecated
public static Table ticket(String ticket) {
return of(TicketTable.of(ticket));
return of(TicketTable.of(ticket.getBytes(StandardCharsets.UTF_8)));
}

/**
Expand Down
8 changes: 6 additions & 2 deletions engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@
import io.deephaven.sql.TableInformation;
import io.deephaven.util.annotations.ScriptApi;

import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.stream.Collectors;

/**
* Experimental SQL execution. Subject to change.
Expand Down Expand Up @@ -64,14 +64,18 @@ private static TableSpec parseSql(String sql, Map<String, Table> scope, Map<Tick
return SqlAdapter.parseSql(sql, scope(scope, out));
}

private static TicketTable sqlref(String tableName) {
return TicketTable.of(("sqlref/" + tableName).getBytes(StandardCharsets.UTF_8));
}

private static Scope scope(Map<String, Table> scope, Map<TicketTable, Table> out) {
final ScopeStaticImpl.Builder builder = ScopeStaticImpl.builder();
for (Entry<String, Table> e : scope.entrySet()) {
final String tableName = e.getKey();
final Table table = e.getValue();
// The TicketTable can technically be anything unique (incrementing number, random, ...), but for
// visualization purposes it makes sense to use the (already unique) table name.
final TicketTable spec = TicketTable.of("sqlref/" + tableName);
final TicketTable spec = sqlref(tableName);
final List<String> qualifiedName = List.of(tableName);
final TableHeader header = adapt(table.getDefinition());
builder.addTables(TableInformation.of(qualifiedName, header, spec));
Expand Down
4 changes: 3 additions & 1 deletion java-client/flight-examples/src/main/java/io/deephaven/client/examples/DoPutSpray.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import picocli.CommandLine.Command;
import picocli.CommandLine.Parameters;

import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -56,7 +57,8 @@ public Void call() throws Exception {
.addShutdownHook(new Thread(() -> onShutdown(scheduler, sourceFactory.managedChannel())));

try (final FlightSession sourceSession = sourceFactory.newFlightSession()) {
try (final TableHandle sourceHandle = sourceSession.session().execute(TicketTable.of(ticket))) {
try (final TableHandle sourceHandle =
sourceSession.session().execute(TicketTable.of(ticket.getBytes(StandardCharsets.UTF_8)))) {
for (ConnectOptions other : connects.subList(1, connects.size())) {
final Factory otherFactory = FlightSessionFactoryConfig.builder()
.clientConfig(other.config())
Expand Down
12 changes: 8 additions & 4 deletions qst/src/main/java/io/deephaven/qst/TableCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.deephaven.qst.table.TicketTable;
import io.deephaven.qst.table.TimeTable;

import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.time.Instant;
import java.util.Arrays;
Expand Down Expand Up @@ -245,18 +246,21 @@ default TABLE merge(TABLE[] tables) {
}

/**
* Equivalent to {@code of(TicketTable.of(ticket))}.
* Create a ticket table with the UTF-8 bytes from the {@code ticket} string. Equivalent to
* {@code of(TicketTable.of(ticket.getBytes(StandardCharsets.UTF_8)))}.
*
* @param ticket the ticket string
* @return the ticket table
* @see TicketTable#of(String)
* @see TicketTable#of(byte[])
* @deprecated prefer {@link #ticket(byte[])} or other explicit methods on {@link TicketTable}
*/
@Deprecated
default TABLE ticket(String ticket) {
return of(TicketTable.of(ticket));
return of(TicketTable.of(ticket.getBytes(StandardCharsets.UTF_8)));
}

/**
* Equivalent to {@code of(TicketTable.of(ticket))}.
* Create a ticket table with the {@code ticket} bytes. Equivalent to {@code of(TicketTable.of(ticket))}.
*
* @param ticket the ticket
* @return the ticket table
Expand Down
23 changes: 16 additions & 7 deletions qst/src/main/java/io/deephaven/qst/table/TableSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,7 @@
import io.deephaven.qst.TableCreator.TableToOperations;
import org.immutables.value.Value.Derived;

import java.io.BufferedInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.charset.StandardCharsets;
import java.util.Collection;

/**
Expand Down Expand Up @@ -57,10 +52,24 @@ static TableSpec of(TableCreationLogic logic) {
return logic.create(TableCreatorImpl.INSTANCE);
}

/**
* Create a ticket table with the UTF-8 bytes from the {@code ticket} string.
*
* @param ticket the ticket
* @return the ticket table
* @deprecated prefer {@link #ticket(byte[])}
*/
@Deprecated
static TicketTable ticket(String ticket) {
return TicketTable.of(ticket);
return TicketTable.of(ticket.getBytes(StandardCharsets.UTF_8));
}

/**
* Create a ticket table with the {@code ticket} bytes.
*
* @param ticket the ticket
* @return the ticket table
*/
static TicketTable ticket(byte[] ticket) {
return TicketTable.of(ticket);
}
Expand Down
6 changes: 4 additions & 2 deletions qst/src/main/java/io/deephaven/qst/table/TicketTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ public static TicketTable of(byte[] ticket) {
*
* @param ticket the ticket string
* @return the ticket table
* @deprecated prefer to be explicit and either use {@link #of(byte[])} or {@link #fromQueryScopeField(String)}
*/
@Deprecated
Comment on lines +36 to +38
Copy link
Member Author

Choose a reason for hiding this comment

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

@rbasralian brought up point; should we prefer to add a more explicitly named fromUtf8 so users can still have an un-deprecated call to this feature? I do think that of(String) made it too easy to mess up, but there may still be general value in the function?

Copy link
Member

Choose a reason for hiding this comment

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

I'd vote no, String.toBytes(StandardCharsets.UTF8) or whatnot is right there (assuming they want utf8 and not ascii etc...).

At least not without some feedback requesting this.

public static TicketTable of(String ticket) {
return ImmutableTicketTable.of(ticket.getBytes(StandardCharsets.UTF_8));
}
Expand All @@ -45,7 +47,7 @@ public static TicketTable of(String ticket) {
* @return the ticket table
*/
public static TicketTable fromQueryScopeField(String fieldName) {
return of("s/" + fieldName);
return of(("s/" + fieldName).getBytes(StandardCharsets.UTF_8));
}

/**
Expand All @@ -56,7 +58,7 @@ public static TicketTable fromQueryScopeField(String fieldName) {
* @return the ticket table
*/
public static TicketTable fromApplicationField(String applicationId, String fieldName) {
return of("a/" + applicationId + "/f/" + fieldName);
return of(("a/" + applicationId + "/f/" + fieldName).getBytes(StandardCharsets.UTF_8));
}

/**
Expand Down
17 changes: 11 additions & 6 deletions sql/src/test/java/io/deephaven/sql/SqlAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import java.io.IOException;
import java.net.URISyntaxException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
Expand Down Expand Up @@ -349,9 +350,13 @@ private static Scope scope() {
return ScopeStaticImpl.empty();
}

private static TicketTable scan(String name) {
return TicketTable.of(("scan/" + name).getBytes(StandardCharsets.UTF_8));
}

private static Scope scope(String name, TableHeader header) {
return ScopeStaticImpl.builder()
.addTables(TableInformation.of(List.of(name), header, TicketTable.of("scan/" + name)))
.addTables(TableInformation.of(List.of(name), header, scan(name)))
.build();
}

Expand All @@ -360,8 +365,8 @@ private static Scope scope(
String name2, TableHeader header2) {
return ScopeStaticImpl.builder()
.addTables(
TableInformation.of(List.of(name1), header1, TicketTable.of("scan/" + name1)),
TableInformation.of(List.of(name2), header2, TicketTable.of("scan/" + name2)))
TableInformation.of(List.of(name1), header1, scan(name1)),
TableInformation.of(List.of(name2), header2, scan(name2)))
.build();
}

Expand All @@ -371,9 +376,9 @@ private static Scope scope(
String name3, TableHeader header3) {
return ScopeStaticImpl.builder()
.addTables(
TableInformation.of(List.of(name1), header1, TicketTable.of("scan/" + name1)),
TableInformation.of(List.of(name2), header2, TicketTable.of("scan/" + name2)),
TableInformation.of(List.of(name3), header3, TicketTable.of("scan/" + name3)))
TableInformation.of(List.of(name1), header1, scan(name1)),
TableInformation.of(List.of(name2), header2, scan(name2)),
TableInformation.of(List.of(name3), header3, scan(name3)))
.build();
}
}
Loading