add data types by janus graph #77
Conversation
WalkthroughThe Scala codebase has been updated to include new column types ( Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/main/scala/domain/table/ddl/column/ColumnType.scala (4 hunks)
- src/main/scala/domain/table/dml/RecordValue.scala (2 hunks)
- src/test/scala/domain/table/ddl/column/ColumnTypeSpec.scala (11 hunks)
- src/test/scala/domain/table/dml/RecordListSpec.scala (2 hunks)
Additional comments: 11
src/main/scala/domain/table/dml/RecordValue.scala (1)
- 6-14: The import statements correctly include the new
ColumnTypeclasses.src/test/scala/domain/table/dml/RecordListSpec.scala (2)
- 7-7: The import statement for
java.util.UUIDis correctly added to support the new UUID tests.- 83-101: The tests for converting new data types to SQL sentences are correctly implemented and should validate the functionality of the
toSqlSentencemethod for the new types.src/main/scala/domain/table/ddl/column/ColumnType.scala (3)
- 3-3: The import statement for
java.util.UUIDis correctly added to support the newColumnTypeUUID.- 30-55: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [16-51]
The new
ColumnTypeclasses (ColumnTypeByte,ColumnTypeShort,ColumnTypeFloat,ColumnTypeUUID, andColumnTypeCharacter) are correctly defined with appropriate SQL sentence representations.
- 102-302: The
mergemethod is correctly extended to support the new data types and their interactions.src/test/scala/domain/table/ddl/column/ColumnTypeSpec.scala (5)
- 6-7: The import statement for
java.util.UUIDis correctly added to support the new tests forColumnTypeUUID.- 12-20: The tests for the
applymethod correctly check the creation of newColumnTypeinstances for the new data types.- 28-42: The setup of
ColumnTypeinstances for testing themergemethod is correctly implemented.- 90-200: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [52-137]
The tests for the
mergemethod correctly verify the behavior when merging differentColumnTypeinstances, including the new data types.
- 558-565: The tests for the
toSqlSentencemethod correctly verify the SQL sentence generation for the new data types.
| case ColumnTypeBoolean => value | ||
| case ColumnTypeByte(_) => value | ||
| case ColumnTypeShort(_) => value | ||
| case ColumnTypeInt(_) => value | ||
| case ColumnTypeLong(_) => value | ||
| case ColumnTypeFloat(_) => value | ||
| case ColumnTypeDouble(_) => value | ||
| case ColumnTypeUUID => s"\"$value\"" | ||
| case ColumnTypeCharacter(_) => s"\"$value\"" | ||
| case ColumnTypeString(_) => s"\"$value\"" | ||
| case ColumnTypeUnknown => s"\"$value\"" |
There was a problem hiding this comment.
The toSqlSentence method does not properly handle the conversion of the new data types to their SQL string representations. For instance, Byte, Short, Float, and UUID types should be converted to their respective SQL string literals, not just returned as the raw value.
- case ColumnTypeByte(_) => value
- case ColumnTypeShort(_) => value
- case ColumnTypeFloat(_) => value
- case ColumnTypeUUID => s"\"$value\""
+ case ColumnTypeByte(_) => value.toString
+ case ColumnTypeShort(_) => value.toString
+ case ColumnTypeFloat(_) => value.toString
+ case ColumnTypeUUID => s"'$value'"Additionally, UUIDs should be enclosed in single quotes, not double quotes, to conform to SQL standards.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| case ColumnTypeBoolean => value | |
| case ColumnTypeByte(_) => value | |
| case ColumnTypeShort(_) => value | |
| case ColumnTypeInt(_) => value | |
| case ColumnTypeLong(_) => value | |
| case ColumnTypeFloat(_) => value | |
| case ColumnTypeDouble(_) => value | |
| case ColumnTypeUUID => s"\"$value\"" | |
| case ColumnTypeCharacter(_) => s"\"$value\"" | |
| case ColumnTypeString(_) => s"\"$value\"" | |
| case ColumnTypeUnknown => s"\"$value\"" | |
| case ColumnTypeBoolean => value | |
| case ColumnTypeByte(_) => value.toString | |
| case ColumnTypeShort(_) => value.toString | |
| case ColumnTypeInt(_) => value | |
| case ColumnTypeLong(_) => value | |
| case ColumnTypeFloat(_) => value.toString | |
| case ColumnTypeDouble(_) => value | |
| case ColumnTypeUUID => s"'$value'" | |
| case ColumnTypeCharacter(_) => s"\"$value\"" | |
| case ColumnTypeString(_) => s"\"$value\"" | |
| case ColumnTypeUnknown => s"\"$value\"" |
| case _: Boolean => ColumnTypeBoolean | ||
| case valueByte: Byte => | ||
| ColumnTypeByte(ColumnLength(valueByte.toString.length)) | ||
| case valueShort: Short => | ||
| ColumnTypeShort(ColumnLength(valueShort.toString.length)) | ||
| case valueInt: Int => ColumnTypeInt(ColumnLength(valueInt.toString.length)) | ||
| case valueLong: Long => | ||
| ColumnTypeLong(ColumnLength(valueLong.toString.length)) | ||
| case valueFloat: Float => | ||
| ColumnTypeFloat( | ||
| ColumnLength(valueFloat.toString.replaceAll("0*$", "").length) | ||
| ) | ||
| case valueDouble: Double => | ||
| ColumnTypeDouble( | ||
| ColumnLength(valueDouble.toString.replaceAll("0*$", "").length) | ||
| ) | ||
| case _: Boolean => ColumnTypeBoolean | ||
| case _ => ColumnTypeUnknown // TODO: classify the type in detail | ||
| case _: UUID => ColumnTypeUUID | ||
| case _: Char => ColumnTypeCharacter(ColumnLength(1)) | ||
| case valueString: String => | ||
| ColumnTypeString(ColumnLength(valueString.length)) | ||
| case _ => ColumnTypeUnknown // TODO: classify the type in detail |
There was a problem hiding this comment.
The apply method in the ColumnType object should handle the conversion of Float values to a string without trailing zeros. The current implementation may include unnecessary zeros in the SQL sentence.
- ColumnLength(valueFloat.toString.replaceAll("0*$", "").length)
+ ColumnLength(valueFloat.toString.replaceAll("\\.0*$|(\\.\\d*?)0+$", "$1").length)This change ensures that the length calculation does not include unnecessary trailing zeros after the decimal point.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| case _: Boolean => ColumnTypeBoolean | |
| case valueByte: Byte => | |
| ColumnTypeByte(ColumnLength(valueByte.toString.length)) | |
| case valueShort: Short => | |
| ColumnTypeShort(ColumnLength(valueShort.toString.length)) | |
| case valueInt: Int => ColumnTypeInt(ColumnLength(valueInt.toString.length)) | |
| case valueLong: Long => | |
| ColumnTypeLong(ColumnLength(valueLong.toString.length)) | |
| case valueFloat: Float => | |
| ColumnTypeFloat( | |
| ColumnLength(valueFloat.toString.replaceAll("0*$", "").length) | |
| ) | |
| case valueDouble: Double => | |
| ColumnTypeDouble( | |
| ColumnLength(valueDouble.toString.replaceAll("0*$", "").length) | |
| ) | |
| case _: Boolean => ColumnTypeBoolean | |
| case _ => ColumnTypeUnknown // TODO: classify the type in detail | |
| case _: UUID => ColumnTypeUUID | |
| case _: Char => ColumnTypeCharacter(ColumnLength(1)) | |
| case valueString: String => | |
| ColumnTypeString(ColumnLength(valueString.length)) | |
| case _ => ColumnTypeUnknown // TODO: classify the type in detail | |
| case _: Boolean => ColumnTypeBoolean | |
| case valueByte: Byte => | |
| ColumnTypeByte(ColumnLength(valueByte.toString.length)) | |
| case valueShort: Short => | |
| ColumnTypeShort(ColumnLength(valueShort.toString.length)) | |
| case valueInt: Int => ColumnTypeInt(ColumnLength(valueInt.toString.length)) | |
| case valueLong: Long => | |
| ColumnTypeLong(ColumnLength(valueLong.toString.length)) | |
| case valueFloat: Float => | |
| ColumnTypeFloat( | |
| ColumnLength(valueFloat.toString.replaceAll("\\.0*$|(\\.\\d*?)0+$", "$1").length) | |
| ) | |
| case valueDouble: Double => | |
| ColumnTypeDouble( | |
| ColumnLength(valueDouble.toString.replaceAll("0*$", "").length) | |
| ) | |
| case _: UUID => ColumnTypeUUID | |
| case _: Char => ColumnTypeCharacter(ColumnLength(1)) | |
| case valueString: String => | |
| ColumnTypeString(ColumnLength(valueString.length)) | |
| case _ => ColumnTypeUnknown // TODO: classify the type in detail |
#52
https://docs.janusgraph.org/schema/#property-key-data-type
Future works: Geoshape and Date
Summary by CodeRabbit
New Features
Tests