-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use 'Numeric' instead of 'Float' as BQ column type when field type in the json schema is number #161
Use 'Numeric' instead of 'Float' as BQ column type when field type in the json schema is number #161
Conversation
import com.snowplowanalytics.iglu.schemaddl.jsonschema.properties.NumberProperty | ||
import com.snowplowanalytics.iglu.schemaddl.jsonschema.Schema | ||
|
||
object decimals { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be private to schemaddl?
@@ -0,0 +1,28 @@ | |||
package com.snowplowanalytics.iglu.schemaddl.jsonschema.suggestion | |||
|
|||
object baseTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be private to schemaddl?
@@ -42,27 +42,27 @@ object Field { | |||
case Some(types) if types.possiblyWithNull(CommonProperties.Type.Object) => | |||
val subfields = topSchema.properties.map(_.value).getOrElse(Map.empty) | |||
if (subfields.isEmpty) { | |||
Suggestion.finalSuggestion(topSchema, required)(name) | |||
Suggestion.finalSuggestion(topSchema, required || types.nullable)(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in #154 I'm fairly sure this should be required && !types.nullable
. To settle this disagreement I think we need a unit test that only passes with one version of the code.
object baseTypes { | ||
sealed trait BaseType extends Product with Serializable | ||
|
||
object BaseType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it BaseNumericType
? Or NumericType
?
else if ((scale == 0) && (max <= Long.MaxValue)) BaseType.Int64 | ||
else BaseType.Decimal(max.precision - max.scale + scale, scale) | ||
|
||
Some(NullableWrapper.fromBool(t, nullable)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place where you ever call NullableWrapper.fromBool
. I think I would prefer writing it out in full here, and then remove the fromBool
function.
if (nullable) Some(NullableWrapper.NullableValue(t))
else Some(NullableWrapper.NotNullValue(t))
|
||
object NullableWrapper { | ||
|
||
case class NullableValue(`type`: BaseType) extends NullableWrapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In schema-ddl, we tend to use the word value
only when there is a real data value (e.g. 42
or "hello"
). If there is no data value, we tend to use the words like type
or field
.
f0997c2
to
b481d87
Compare
mixed up the branches on this one. Continued in #162. |
No description provided.