apache / apache/polaris

PropertyMapEventListener is incomplete / underspecified

Open
#2,630 5 comments 0 reactions 1 assignee Claimed by @vchag View on GitHub
bug
Dominant language
Java
Stars
2.1k
Forks
522
Avg merge
1d 22h
Merged PRs (30d)
137

Description

### Describe the bug

Currently, `PropertyMapEventListener` implements only one method from `PolarisEventListener` – all others are ignored. It is unclear if this is on purpose.

Furthermore:

- The API is not ideal: `transformAndSendEvent` takes a `HashMap` parameter, and it is unclear whether the requirement for a `HashMap` (instead of `Map`) is legit.
- The only implemented method – `onAfterRefreshTable` does not serialize all the event fields. Again, it is unclear if this is by design or an oversight.

And finally, the whole point of having an abstract class that transforms events into Maps, only to have the Maps transformed into Json (cf. `AwsCloudWatchEventListener`) seems unnecessary.

If the target serialization format is Json, it would be beneficial to provide Jackson serialization info in the event types themselves, e.g.:

```java
public record AfterRefreshTableEvent(
@JsonProperty("catalog_name")
String catalogName,
@JsonProperty("table_identifier")
TableIdentifier tableIdentifier)
implements PolarisEvent {}
```

Or alternatively:

```java
@JsonNaming(SnakeCaseStrategy.class)
public record AfterRefreshTableEvent(
String catalogName,
TableIdentifier tableIdentifier)
implements PolarisEvent {}
```

The above would be naturally serialized as:

```json
{
"catalog_name" : "test_catalog",
"table_identifier" : {
"namespace" : [ "test_namespace" ],
"name" : "test_table"
}
}
```

Without the need for `PropertyMapEventListener`.

`PropertyMapEventListener` will also become problematic for larger payloads e.g. `TableMetadata` as the serialization logic is not well-defined (which fields to include? which case? which order?)

### To Reproduce

_No response_

### Actual Behavior

_No response_

### Expected Behavior

_No response_

### Additional context

_No response_

### System information

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.