Core: add parsers for events and unbounded expressions #4308

kunal0829 · 2022-03-10T20:10:36Z

In this PR, the Iceberg events are now serializable into JSON by introducing an EventParser along with the addition of ExpressionParser.

* Event Parser and Logging Added Expression parser in event parser and adding error logging for notify. * Removed unused imports * Update scan test logic and SNS error message Combine calls for scan event logic in test. Add topic name in error message for SNS topic. * Remove unused import Did not remove Tablescan import after changing scan to one inline operation * Fix error throwing

Co-authored-by: Kunal Bhargava <kunbharg@amazon.com>

* Added Event Types in Json * Changed Reflection Initialization * Changed Event Type Input in Json * Fixed Listener Initialization

* Added Event Types in Json * Changed Reflection Initialization * Changed Event Type Input in Json * Fixed Listener Initialization * Added Partial fromJson in ExpressionParser Still missing Literal serialization. * Added Partial fromJson in ExpressionParser Still missing Literal serialization. * Added Literal Serialization * Fixed DecimalLiteral Serialization * Changed to Iceberg Private Classes (Original)

jackye1995 · 2022-03-10T20:48:17Z

Can we limit this PR to only event parsers? The purpose is to breakdown the draft, not republish it

jackye1995 · 2022-03-10T22:01:35Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+    }
+  }
+
+  @SuppressWarnings("CyclomaticComplexity")


this is not needed?

jackye1995 · 2022-03-10T22:02:02Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+    }
+  }
+
+  public static Predicate<?, ?> fromJsonToPredicate(JsonNode json, String predicateType) {


please keep all methods private except for the main one for Expression

jackye1995 · 2022-03-10T22:02:46Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+  public static Predicate<?, ?> fromJsonToPredicate(JsonNode json, String predicateType) {
+    if (UNBOUNDED_PREDICATE.equals(predicateType)) {
+      return fromJsonUnboundPredicate(json);
+    } else if (BOUNDED_LITERAL_PREDICATE.equals(predicateType)) {


since we are only targeting unbounded, we can just use a generic else block saying we don't support bounded expression yet

jackye1995 · 2022-03-10T22:03:56Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+
+  public static void toJson(And expression, JsonGenerator generator) throws IOException {
+    generator.writeStartObject();
+    generator.writeStringField(TYPE, AND);


here can we reuse the word "operation"? Because phrases like "and" and "or" are operations

jackye1995 · 2022-03-10T22:04:32Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+    generator.writeStringField(OPERATION, predicate.op().name().toLowerCase());
+    generator.writeFieldName(TERM);
+    toJson(predicate.term(), generator);
+    if (!(ONE_INPUTS.contains(predicate.op()))) {


redundant brackets after !

jackye1995 · 2022-03-10T22:05:02Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+    generator.writeEndObject();
+  }
+
+  public static void toJson(List<Literal<?>> literals, JsonGenerator generator) throws IOException {


this method seems unnecessary, can be combined to parent method, also can use enhanced for loop

jackye1995 · 2022-03-10T22:05:45Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+  private static final String LEFT_OPERAND = "left-operand";
+  private static final String RIGHT_OPERAND = "right-operand";
+  private static final String OPERAND = "operand";
+  private static final String AND = "and";


these can just use operation.name().toLowerCase(), no need to define extra variables

jackye1995 · 2022-03-10T22:07:00Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+      return True.INSTANCE;
+    } else if (FALSE.equals(expressionType)) {
+      return False.INSTANCE;
+    } else if (PREDICATE_TYPES.contains(expressionType)) {


I think we don't need a set of predicate types, we just need to have a single else block, delegating all the rest of the conditions to fromJsonToPredicate(json, expressionType);

jackye1995 · 2022-03-10T22:08:22Z

core/src/main/java/org/apache/iceberg/util/EventParser.java

+
+public class EventParser {
+  private static final String EVENT_TYPE = "event-type";
+


nit: extra newline

jackye1995 · 2022-03-10T22:09:47Z

core/src/main/java/org/apache/iceberg/util/EventParser.java

+    }
+  }
+
+  public static ScanEvent fromJsonToScanEvent(JsonNode json) {


keep methods below private

jackye1995 · 2022-03-10T22:10:23Z

core/src/test/java/org/apache/iceberg/expressions/TestExpressionParser.java

+  }
+
+  @Test
+  public void testParserBothWays() {


nit: "roundTrip" instead of "bothWays"

rdblue · 2022-03-11T23:17:26Z

Looks like this is proposing JSON serialization. Can you write up the proposal for those formats? It is much easier to review the proposed formats that way, rather than looking at code that does it.

rdblue · 2022-03-11T23:47:11Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+  private static final String NOT = "not";
+  private static final String TRUE = "true";
+  private static final String FALSE = "false";
+  private static final String UNBOUNDED_PREDICATE = "unbounded-predicate";


The term is "unbound", meaning that the expression has not been bound to a particular incoming struct/row type, not "unbounded".

rdblue · 2022-03-11T23:47:31Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+  private static final String FALSE = "false";
+  private static final String UNBOUNDED_PREDICATE = "unbounded-predicate";
+  private static final String BOUNDED_LITERAL_PREDICATE = "bounded-literal-predicate";
+  private static final String BOUNDED_SET_PREDICATE = "bounded-set-predicate";


Do we need to serialize bound predicates?

rdblue · 2022-03-11T23:49:06Z

core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

+    }
+  }
+
+  public static void toJson(Expression expression, JsonGenerator generator) throws IOException {


Iceberg separates the recursive logic to process a tree structure from the logic to do something by using visitors. I think that this should use an in-order expression visitor to produce a JSON representation.

rdblue · 2022-03-11T23:51:27Z

core/src/main/java/org/apache/iceberg/util/EventParser.java

+import org.apache.iceberg.expressions.Expression;
+import org.apache.iceberg.expressions.ExpressionParser;
+
+public class EventParser {


What kind of event is this parser for? It appears to accept Object in toJson, which is suspicious.

rdblue · 2022-03-11T23:51:52Z

core/src/main/java/org/apache/iceberg/util/EventParser.java

+      if (pretty) {
+        generator.useDefaultPrettyPrinter();
+      }
+      if (event instanceof ScanEvent) {


We add newlines after control flow blocks and the following statement.

rdblue · 2022-03-11T23:52:04Z

core/src/main/java/org/apache/iceberg/util/EventParser.java

+        toJson((CreateSnapshotEvent) event, generator);
+      } else if (event instanceof IncrementalScanEvent) {
+        toJson((IncrementalScanEvent) event, generator);
+      }


If the event is not handled, this will just fail?

Made Parser function private that aren't called outside the classes and changed Operation type to "operation" instead of "type"

rdblue · 2022-04-10T18:51:57Z

@kunal0829, can you write up a doc that summarizes the proposed serialization formats?

kunal0829 · 2022-04-11T07:18:44Z

@rdblue Here is an example write up that explains the proposed format: https://docs.google.com/document/d/1CvOfsVAMezqQyyAwIn0qfew9EvlX-5PIvKGHPzlHldA/edit?usp=sharing

Please let me know if you have any questions about the proposed format

…829/iceberg into listener-and-event-parser

rdblue · 2022-04-24T18:51:34Z

I've not been able to comment on the doc since it isn't opened up for discussion. I'll add a few of the comments here instead.

My main concern about the proposed JSON format is that it is needlessly verbose and relies heavily on the JVM classes rather than representing the expressions more directly. For example, it is scoped to only unbound expressions, but still uses "unbound-predicate" and "named-reference". It also uses "left-operand" and "right-operand". I think it would be better to use fewer words and make better use of "type".

Each node in the expression tree should be represented as a JSON object. Those are distinguished by the type field: that can have the following values: and, or, not, eq, not-eq, in, not-in, lt, lt-eq, gt, gt-eq, starts-with, not-starts-with, is-null, not-null, is-nan, not-nan, reference, transform, and literal.

Then there is some specialization by type:

and and or must have child expressions named left and right
not must have a child expression named child
All predicates must have a term expression that is either a transform or a reference
transform must have a term expression that is either a transform or a reference and a transform field that is a transform string ("identity", "bucket[16]", etc.)
Set predicates (in and not-in) must have a JSON array called values of literals
Comparison predicates (eq, etc.) must have a value that is a literal
reference must have a name that is a JSON string
literal must have a value field
Literal values must use the JSON value serialization defined in Docs: Default value support feature specification #4301 (structs, lists, and maps are not allowed)

I would also allow the reference to be replaced with a string field name, and a literal to be replaced with the value when nested.

I think this format is more direct:

# example 1: event_ts = 2022-04-24T11:42:12.578391 OR bucket(16, id) IN (0, 1)
{
  "type": "or",
  "left": {
    "type": "eq",
    "term": "event_ts", # equivalent to { "type": "reference", "name": "event_ts" }
    "value": "2022-04-24T11:42:12.578391" # equivalent to { "type": "literal", "value": "2022-04-24T11:42:12.578391" }
  }
  "right": {
    "type": "in"
    "term": {
      "type": "transform",
      "transform": "bucket[16]",
      "term": "id"
    },
    "values": [
      { "type": "literal", "value": 0 },
      1
    ]
  }
}

# example 2: true
{ "type": "literal", "value": true }

rdblue · 2022-08-22T19:27:51Z

@kunal0829 and @jackye1995, an expression JSON parser was just merged in #5602.

kunal0829 and others added 15 commits March 10, 2022 13:51

SNS Implementation for Event Notification

df0e467

Expression Parser (toJson) (apache#3)

1edc3ec

Add SQSListener for automatic integration test (apache#6)

0c6a0bc

Fix integration test and cleanup codebase

29aa16e

Catalog Listener (apache#7) (apache#9)

c57789f

Co-authored-by: Kunal Bhargava <kunbharg@amazon.com>

fix rebase

f1cbe93

refactor listener and catalog initialization logics

ed16ef5

Added Event Types in Json (apache#10)

da57f74

* Added Event Types in Json * Changed Reflection Initialization * Changed Event Type Input in Json * Fixed Listener Initialization

add some fixes to parser

b969fb3

Updated FixedLiteral and Testing

55c1505

Added fromJson Event Serialization

e61ce99

Fixed BaseLiteral

0aa2f8c

Fixed Space

b545ade

github-actions bot added API AWS build core hive NESSIE labels Mar 10, 2022

kunal0829 changed the title ~~AWS: Add Support for SNS and SQS Listeners~~ Core: EventParser and ExpressionParser Mar 10, 2022

Removed Listener Implementations

39dee63

kunal0829 changed the title ~~Core: EventParser and ExpressionParser~~ Core: add parsers for events and unbounded expressions Mar 10, 2022

jackye1995 reviewed Mar 10, 2022

View reviewed changes

rdblue reviewed Mar 11, 2022

View reviewed changes

Updated Parser

d990df1

Made Parser function private that aren't called outside the classes and changed Operation type to "operation" instead of "type"

Updated Bound Predicates Json Serialization

2bde5a8

kunal0829 added 2 commits April 11, 2022 00:20

Updated Unbound Predicates Json Serialization

636c9b8

Merge branch 'listener-and-event-parser' of https://github.com/kunal0…

73ec648

…829/iceberg into listener-and-event-parser

rdblue added a commit to rdblue/iceberg that referenced this pull request Aug 14, 2022

Use an expression visitor and serialization from apache#4308.

aebc07c

rdblue added a commit to rdblue/iceberg that referenced this pull request Aug 14, 2022

Use an expression visitor and serialization from apache#4308.

c291c01

rdblue added a commit to rdblue/iceberg that referenced this pull request Aug 14, 2022

Use an expression visitor and serialization from apache#4308.

46ee744

rdblue mentioned this pull request Aug 14, 2022

Use an expression visitor and serialization from #4308. nastra/iceberg#25

Closed

rdblue added a commit to rdblue/iceberg that referenced this pull request Aug 14, 2022

Use an expression visitor and serialization from apache#4308.

d02d211

rdblue added a commit to rdblue/iceberg that referenced this pull request Aug 14, 2022

Use an expression visitor and serialization from apache#4308.

e420439

rdblue mentioned this pull request Aug 21, 2022

Core: Add expression JSON parser #5602

Merged

rdblue closed this in #5602 Aug 22, 2022


		public class EventParser {
		private static final String EVENT_TYPE = "event-type";

Core: add parsers for events and unbounded expressions #4308

Core: add parsers for events and unbounded expressions #4308

Uh oh!

Conversation

kunal0829 commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackye1995 commented Mar 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Mar 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Apr 10, 2022

Uh oh!

kunal0829 commented Apr 11, 2022

Uh oh!

rdblue commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kunal0829 commented Mar 10, 2022 •

edited

Loading

rdblue Mar 11, 2022 •

edited

Loading

rdblue commented Apr 24, 2022 •

edited

Loading