You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/09/19 16:09:27 UTC
[I] PropertyMapEventListener is incomplete / underspecified [polaris]
adutra opened a new issue, #2630:
URL: https://github.com/apache/polaris/issues/2630
### 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_
--
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: issues-unsubscribe@polaris.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] PropertyMapEventListener is incomplete / underspecified [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on issue #2630:
URL: https://github.com/apache/polaris/issues/2630#issuecomment-3370445403
Thanks @vchag, the issue is assigned to you!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] PropertyMapEventListener is incomplete / underspecified [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on issue #2630:
URL: https://github.com/apache/polaris/issues/2630#issuecomment-3380643469
@vchag Indeed I think we need to get rid of `PropertyMapEventListener`.
I'm not sure that Jackson Annotations on the events themselves would hinder Protobuf or Avro serialization, but I agree that mixins could be a nice alternative. I don't have strong opinions on the "how" as long as we agree on the "why".
Cc @adnanhemani
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] PropertyMapEventListener is incomplete / underspecified [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on issue #2630:
URL: https://github.com/apache/polaris/issues/2630#issuecomment-3369222286
@adutra I'm happy to take this up.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] PropertyMapEventListener is incomplete / underspecified [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on issue #2630:
URL: https://github.com/apache/polaris/issues/2630#issuecomment-3413890470
Well, yes using Jackson Annotation on events themselves won't hinder Protobuf of Avro Serialization. But, it would certainly introduce annotation clutter.
Mixins provide clear separation between Domain login and Serialization logic.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] PropertyMapEventListener is incomplete / underspecified [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on issue #2630:
URL: https://github.com/apache/polaris/issues/2630#issuecomment-3379456764
Thanks @adutra ! ... a few thoughts; curious for your take.
1. Currently `PropertyMapEventListener` the name makes it sound like a generic event listener but it only overrides `onAfterRefreshTable`, which does look odd. If the intent was to only care about `AfterRefreshTableEvent` then I think we should rename `PropertyMapEventListener` to `AfterRefreshTableEventListener`, any thoughts?
2. Properties parameter `HashMap` -> `Map`: This becomes moot if we agree on #3.
3. `events` -> `maps` -> `JSON`: Since the target serialization format is Json, the extra hope is unnecessary. Direct Jackson annotations will couple the event classes to JSON and hinder future Avro/Protobuf support. let’s use Jackson Mixins instead to keep the models serializer agnostic while still producing consistent JSON?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org