You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/08/23 10:19:26 UTC
[GitHub] [beam] nielsbasjes commented on a change in pull request #14729: [BEAM-9379] Update calcite to 1.26
nielsbasjes commented on a change in pull request #14729:
URL: https://github.com/apache/beam/pull/14729#discussion_r693824426
##########
File path: sdks/java/extensions/sql/src/main/codegen/includes/parserImpls.ftl
##########
@@ -146,10 +146,10 @@ Schema.Field Field() :
* ( LOCATION location_string )?
* ( TBLPROPERTIES tbl_properties )?
*/
-SqlCreate SqlCreateExternalTable() :
+SqlCreate SqlCreateExternalTable(Span s, boolean replace) :
{
- final Span s = Span.of();
- final boolean replace = false;
+<#-- final Span s = Span.of();-->
Review comment:
Remove this obsolete code.
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlCliTest.java
##########
@@ -260,7 +260,7 @@ public void test_time_types() throws Exception {
"INSERT INTO test_table VALUES ("
+ "DATE '2018-11-01', "
+ "TIME '15:23:59', "
- + "TIMESTAMP '2018-07-01 21:26:07.123' )");
+ + "TIMESTAMP '2018-07-01 21:26:07' )");
Review comment:
Why remove the milliseconds from this test case?
If this no longer works then this would imply a backwards compatibility issue to me.
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamTableStatistics.java
##########
@@ -76,6 +77,11 @@ public boolean isKey(ImmutableBitSet columns) {
return false;
}
+ @Override
+ public List<ImmutableBitSet> getKeys() {
+ return Collections.emptyList(); // FIXME BEAM-9379: Is this correct???
Review comment:
I wrote this "FIXME" because I was unsure of the code. Is this correct now? If so remove the comment please.
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/cep/CEPUtils.java
##########
@@ -156,11 +156,11 @@ public static String getRegexFromPattern(RexNode call) {
}
/** Transform the partition columns into serializable CEPFieldRef. */
- public static List<CEPFieldRef> getCEPFieldRefFromParKeys(List<RexNode> parKeys) {
+ public static List<CEPFieldRef> getCEPFieldRefFromParKeys(ImmutableBitSet parKeys) {
ArrayList<CEPFieldRef> fieldList = new ArrayList<>();
- for (RexNode i : parKeys) {
- RexInputRef parKey = (RexInputRef) i;
- fieldList.add(new CEPFieldRef(parKey.getName(), parKey.getIndex()));
+ for (int index : parKeys.asList()) {
+ // FIXME: Don't know where to get a better name.
Review comment:
I wrote this comment a long time ago. Is this the correct name now?
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rule/JoinRelOptRuleCall.java
##########
@@ -53,6 +54,15 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv) {
}
}
+ @Override
+ public void transformTo(
+ RelNode relNode, Map<RelNode, RelNode> map, RelHintsPropagator relHintsPropagator) {
+ if (checker.check(relNode)) {
+ // FIXME: CHECK IF THIS IS CORRECT
Review comment:
Is this correct?
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamMatchRel.java
##########
@@ -465,4 +462,25 @@ public Match copy(
orderKeys,
interval);
}
+
+ @Override
+ public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs) {
+ // FIXME: THIS IS PROBABLY WRONG
+ return new BeamMatchRel(
+ getCluster(),
+ traitSet,
+ input,
+ rowType,
+ pattern,
+ strictStart,
+ strictEnd,
+ patternDefinitions,
+ measures,
+ after,
+ subsets,
+ allRows,
+ partitionKeys,
+ orderKeys,
+ interval);
Review comment:
If this is probably wrong then this must probably be updated :)
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigtable/BigtableTableTestUtils.java
##########
@@ -170,7 +170,7 @@ static void createReadTable(String table, BigtableClientWrapper clientWrapper) {
ImmutableList.of(
column("boolColumn", booleanToByteArray(true)),
column("doubleColumn", doubleToByteArray(5.5)),
- column("longColumn", Longs.toByteArray(10L)),
+ column("longColumn", Ints.toByteArray(10)),
Review comment:
A `longColumn` that contains an `Int` ?
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/planner/RelMdNodeStats.java
##########
@@ -71,16 +72,34 @@ private NodeStats getBeamNodeStats(BeamRelNode rel, RelMetadataQuery mq) {
// wraps the metadata provider with CachingRelMetadataProvider. However,
// CachingRelMetadataProvider checks timestamp before returning previous results. Therefore,
// there wouldn't be a problem in that case.
- List<List> keys =
- mq.map.entrySet().stream()
- .filter(entry -> entry.getValue() instanceof NodeStats)
- .filter(entry -> ((NodeStats) entry.getValue()).isUnknown())
- .map(Map.Entry::getKey)
- .collect(Collectors.toList());
-
- for (List key : keys) {
- mq.map.remove(key);
+ Set<Table.Cell<RelNode, List, Object>> cells = mq.map.cellSet();
+ List<Table.Cell<RelNode, List, Object>> keys = new ArrayList<>(cells.size());
+ for (Table.Cell<RelNode, List, Object> cell : cells) {
+ if (cell == null) {
+ continue;
+ }
+ Object rawValue = cell.getValue();
+ if (!(rawValue instanceof NodeStats)) {
+ continue;
+ }
+ NodeStats nodeStats = (NodeStats) rawValue;
+ if (nodeStats.isUnknown()) {
+ keys.add(cell);
+ }
}
+ // List<Table.Cell<RelNode, List, Object>> keys =
+ // mq.map.cellSet().stream()
+ // .filter(entry -> entry.getValue() instanceof NodeStats)
+ // .filter(entry -> ((NodeStats) entry.getValue()).isUnknown())
+ // .collect(Collectors.toList());
+
+ // === > Task :sdks:java:extensions:sql:compileJava
+ // === error: [dereference.of.nullable] dereference of possibly-null reference
+ // ((NodeStats)entry.getValue())
+ // === .filter(entry -> ((NodeStats) entry.getValue()).isUnknown())
+ // === ^
Review comment:
Old code?
##########
File path: sdks/java/extensions/sql/src/main/codegen/includes/parserImpls.ftl
##########
@@ -160,7 +160,7 @@ SqlCreate SqlCreateExternalTable() :
}
{
- <CREATE> <EXTERNAL> <TABLE> {
+ <EXTERNAL> <TABLE> {
Review comment:
Does this mean a change in the SQL syntax (the removal of the `create` ) ? If so; what are the backwards compatibility effects for user applications?
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java
##########
@@ -201,6 +202,11 @@ public static SqlTypeName toSqlTypeName(FieldType type) {
}
}
+ public static FieldType toFieldType(SqlTypeNameSpec sqlTypeName) {
+ // FIXME: NO CLUE IF THIS IS CORRECT
Review comment:
Is this correct?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org