flink-connector-aws icon indicating copy to clipboard operation
flink-connector-aws copied to clipboard

[FLINK-35022][Connector/DynamoDB] Add TypeInformed DDB Element Converter as default element converter

Open vahmed-hamdy opened this issue 1 year ago • 4 comments

Purpose of the change

Add DynamoDbTypeInformedElementConverter to convert Elements to dynamoDb Sink using its provided type Info.

Verifying this change

This change added tests and can be verified as follows:

  • Added unit tests

Significant changes

(Please check any boxes [x] if the answer is "yes". You can first publish the PR and check them afterwards, for convenience.)

  • [ ] Dependencies have been added or upgraded
  • [ ] Public API has been changed (Public API is any class annotated with @Public(Evolving))
  • [ ] Serializers have been changed
  • [ ] New feature has been introduced
    • If yes, how is this documented? (not applicable / docs / JavaDocs / not documented)

vahmed-hamdy avatar Apr 11 '24 11:04 vahmed-hamdy

Please change commit message to include the component: [FLINK-35022][Connectors/DynamoDB]

dannycranmer avatar Apr 15 '24 09:04 dannycranmer

I have left a comment regarding the need for this on the Jira: https://issues.apache.org/jira/browse/FLINK-35022

dannycranmer avatar Apr 15 '24 09:04 dannycranmer

@dannycranmer Thanks for the feedback, I have addressed your comments. Alternatively I decided to follow an approach similar to RowDataToAttributeValueConverter to avoid using reflection. PTAL

vahmed-hamdy avatar Apr 20 '24 01:04 vahmed-hamdy

@vahmed-hamdy can you also update the docs please?

dannycranmer avatar Apr 25 '24 13:04 dannycranmer

@hlteoh37 / @z3d1k can you please take a look at this one too?

dannycranmer avatar Jun 10 '24 15:06 dannycranmer

@hlteoh37 Thanks, for the feedback, the reason we took this approach is that We are trying to couple it as much as possible with Flink's TypeInfo Class, Using AttributeConverterProvider is closer to DDB's EnhancedType rather than Flink's

vahmed-hamdy avatar Jun 13 '24 15:06 vahmed-hamdy

@hlteoh37 Do you have other comments or could we merge this PR?

vahmed-hamdy avatar Jun 24 '24 10:06 vahmed-hamdy

Hi @vahmed-hamdy, my preference would be for us to refactor the code in DynamoDbTypeInformedElementConverter to use a cleaner structure rather than a long list of if-else. Is this something we need to close urgently?

hlteoh37 avatar Jun 24 '24 10:06 hlteoh37