Skip to content

[BEAM-2310] support new data type: DECIMAL#3168

Closed
xumingming wants to merge 3 commits intoapache:DSL_SQLfrom
xumingming:BEAM-2310-decimal
Closed

[BEAM-2310] support new data type: DECIMAL#3168
xumingming wants to merge 3 commits intoapache:DSL_SQLfrom
xumingming:BEAM-2310-decimal

Conversation

@xumingming
Copy link
Contributor

  1. support new data type: DECIMAL
  2. support encoding/decoding for TIME
  3. add tests all the supported data types.

R: @xumingmin

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 12331d5 on xumingming:BEAM-2310-decimal into ** on apache:DSL_SQL**.

@jbonofre
Copy link
Member

R: @jbonofre

}

instantCoder.encode(value.getWindowStart(), outStream, context.nested());
instantCoder.encode(value.getWindowStart(), outStream, context);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would keep as context.nested(), and only has the last field in coder/decoder as context,
Same for method decode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any criteria for this reason? I don't quite understand the context/context.nested() thing, my current strategy is just to keep consistent in encode/decode.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would follow the pattern to have only the last field as context, and context().nested() with others.

@xumingming
Copy link
Contributor Author

@jbonofre can you take a look at this one?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9ff1a6a on xumingming:BEAM-2310-decimal into ** on apache:DSL_SQL**.

Copy link

@mingmxu mingmxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm resolving conflict and merging.

asfgit pushed a commit that referenced this pull request May 24, 2017
@jbonofre
Copy link
Member

Merged on DSL_SQL branch, you can close this Pull Request. Thanks !

@xumingming
Copy link
Contributor Author

Thanks!

@xumingming xumingming closed this May 24, 2017
@xumingming xumingming deleted the BEAM-2310-decimal branch May 24, 2017 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants