ibzib commented on a change in pull request #14648:
URL: https://github.com/apache/beam/pull/14648#discussion_r621623321
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -120,6 +125,14 @@
@Test
public void testSQLSelectsPayloadContent() throws Exception {
+
+ // Prepare messages to send later
Review comment:
Why do we need to "prepare messages to send later"? We only use the
messages in one place, so I don't see why we need to define messages up front.
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -610,20 +650,31 @@ public void
testSQLReadAndWriteWithSameFlatTableDefinition() throws Exception {
objectsProvider.getPayloadFormat() == null
? ""
: String.format(
- "TBLPROPERTIES '{\"format\": \"%s\"}'",
objectsProvider.getPayloadFormat());
+ "TBLPROPERTIES " + "'{ " + "%s" + "\"format\": \"%s\"" + "}'",
Review comment:
Please remove the string concatenations and make this all a single
string.
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -509,24 +539,32 @@ public void testSQLSelectsPayloadContentFlat() throws
Exception {
@Test
@SuppressWarnings("unchecked")
public void testSQLInsertRowsToPubsubFlat() throws Exception {
+
+ Matcher<PubsubMessage> person1matcher =
Review comment:
Nit: why separate these out into variables? They looked fine where they
were.
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -610,20 +650,31 @@ public void
testSQLReadAndWriteWithSameFlatTableDefinition() throws Exception {
objectsProvider.getPayloadFormat() == null
? ""
: String.format(
- "TBLPROPERTIES '{\"format\": \"%s\"}'",
objectsProvider.getPayloadFormat());
+ "TBLPROPERTIES " + "'{ " + "%s" + "\"format\": \"%s\"" + "}'",
Review comment:
Please remove the string concatenations and make this all a single
string.
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -781,6 +836,63 @@ protected abstract PubsubMessage messageIdName(Instant
timestamp, int id, String
throws Exception;
}
+ private static class PubsubProtoObjectProvider extends PubsubObjectProvider {
+
+ @Override
+ protected String getPayloadFormat() {
+ return "proto";
+ }
+
+ @Override
+ protected PubsubMessage messageIdName(Instant timestamp, int id, String
name) {
+
+ PayloadMessages.SimpleMessage.Builder simpleMessage =
+ PayloadMessages.SimpleMessage.newBuilder();
Review comment:
Nit: you can put setter methods and build() together all on one line.
```java
PayloadMessages.SimpleMessage simpleMessage =
PayloadMessages.SimpleMessage.newBuilder().setId(id).setName(name).build();
```
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -610,20 +650,31 @@ public void
testSQLReadAndWriteWithSameFlatTableDefinition() throws Exception {
objectsProvider.getPayloadFormat() == null
? ""
: String.format(
- "TBLPROPERTIES '{\"format\": \"%s\"}'",
objectsProvider.getPayloadFormat());
+ "TBLPROPERTIES " + "'{ " + "%s" + "\"format\": \"%s\"" + "}'",
+
protoClassParam(PayloadMessages.NameHeightKnowsJSMessage.class.getName()),
+ objectsProvider.getPayloadFormat());
+
String createTableString =
String.format(
"CREATE EXTERNAL TABLE people (\n"
+ "event_timestamp TIMESTAMP, \n"
+ "name VARCHAR, \n"
+ "height INTEGER, \n"
- + "knowsJavascript BOOLEAN \n"
+ + "knows_javascript BOOLEAN \n"
+ ") \n"
+ "TYPE '%s' \n"
+ "LOCATION '%s' \n"
+ "%s",
tableProvider.getTableType(), eventsTopic.topicPath(),
tblProperties);
+ String filteredTblProperties =
+ objectsProvider.getPayloadFormat() == null
+ ? ""
+ : String.format(
+ "TBLPROPERTIES " + "'{ " + "%s" + "\"format\": \"%s\"" + "}'",
Review comment:
Please remove the string concatenations and make this all a single
string.
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -781,6 +836,63 @@ protected abstract PubsubMessage messageIdName(Instant
timestamp, int id, String
throws Exception;
}
+ private static class PubsubProtoObjectProvider extends PubsubObjectProvider {
+
+ @Override
+ protected String getPayloadFormat() {
+ return "proto";
+ }
+
+ @Override
+ protected PubsubMessage messageIdName(Instant timestamp, int id, String
name) {
+
+ PayloadMessages.SimpleMessage.Builder simpleMessage =
+ PayloadMessages.SimpleMessage.newBuilder();
+
+ simpleMessage.setId(id);
+ simpleMessage.setName(name);
+
+ return PubsubTableProviderIT.message(
+ timestamp,
+ simpleMessage.build().toByteArray(),
+ ImmutableMap.of(name, Integer.toString(id)));
+ }
+
+ @Override
+ protected Matcher<PubsubMessage> matcherNames(String name) throws
IOException {
Review comment:
Why is the `name` argument not used at all here?
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -725,6 +776,10 @@ private String payloadFormatParam() {
: String.format("\"format\" : \"%s\", ",
objectsProvider.getPayloadFormat());
}
+ private String protoClassParam(String protoClassName) {
Review comment:
Nit: this helper method replaces`String.format("\"protoClass\" :
\"%s\"", protoClassName)` with `String.format("%s",
protoClassParam(protoClassName))`. The helper method introduces indirection,
which makes the JSON strings harder to read, but doesn't really add much
benefit. So please remove `protoClassParam`.
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/pubsub/PubsubTableProviderIT.java
##########
@@ -781,6 +836,63 @@ protected abstract PubsubMessage messageIdName(Instant
timestamp, int id, String
throws Exception;
}
+ private static class PubsubProtoObjectProvider extends PubsubObjectProvider {
+
+ @Override
+ protected String getPayloadFormat() {
+ return "proto";
+ }
+
+ @Override
+ protected PubsubMessage messageIdName(Instant timestamp, int id, String
name) {
+
+ PayloadMessages.SimpleMessage.Builder simpleMessage =
+ PayloadMessages.SimpleMessage.newBuilder();
Review comment:
Nit: you can call setter methods and build() all together.
```java
PayloadMessages.SimpleMessage simpleMessage =
PayloadMessages.SimpleMessage.newBuilder().setId(id).setName(name).build();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
