Conversation
sahusanket
force-pushed
the
bq-direct-source
branch
from
fe18de9 to
b49f70c
Compare
sahusanket
force-pushed
the
bq-direct-source
branch
from
b49f70c to
45f9d25
Compare
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.lang.reflect.Type; |
There was a problem hiding this comment.
Why was this import removed?
| public static final String SQL_INPUT_FIELDS = "fields"; | ||
| public static final String SQL_INPUT_SCHEMA = "schema"; | ||
| private static final Type LIST_OF_STRINGS_TYPE = new TypeToken<ArrayList<String>>() { | ||
| private static final java.lang.reflect.Type LIST_OF_STRINGS_TYPE = new TypeToken<ArrayList<String>>() { |
There was a problem hiding this comment.
I don't think this fully qualified import is needed. There are no conflicting class names.
| if (condition.length() == 0) { | ||
| condition.append(filter); | ||
| } else { | ||
| condition.append(" and (").append(filter).append(")"); |
There was a problem hiding this comment.
nit: WHERE is uppercase, AND should be as well.
|
|
||
| String partitionFilter = "TIMESTAMP(`_PARTITIONTIME`) >= TIMESTAMP(\"2000-01-01\") and " + | ||
| "TIMESTAMP(`_PARTITIONTIME`) < TIMESTAMP(\"2000-01-01\")"; | ||
| expectedQuery = String.format("SELECT %s FROM `%s.%s.%s` WHERE %s and (%s)", String.join(",", fieldList), |
There was a problem hiding this comment.
nit: The expected query should be a fully defined string, not something that is built using code. This ensures that a bug doesn't also propagate to the test itself.
There was a problem hiding this comment.
You can use the debugger to print the expectedQuery value and replace the string here.
| // Add output for SQL Engine Direct read | ||
| ImmutableMap.Builder<String, String> arguments = new ImmutableMap.Builder<>(); | ||
|
|
||
| List<String> fieldNames = configuredSchema.getFields().stream().map(f -> f.getName()).collect(Collectors.toList()); |
There was a problem hiding this comment.
We had to make a change on the BQ sink to only configure the Pushdown sink when a schema is configured in #1120
Can you add the same here? Essentially, if configuredSchema is null, don't initialize the pushdown sink. Realistically, I don't see how pushdown execution would even work if the schema is not known at runtime.
2 participants
No description provided.