Support table creation of Scalar DB on JDBC in schema-tool #152
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yito88
reviewed
Feb 5, 2021
|
@yito88 Thank you for reviewing this and giving me the great advices! I will modify the patch based on them and push a new commit for it. Thanks. |
feeblefakie
reviewed
Feb 8, 2021
| (= rdb-engine :sql-server) "BIT" | ||
| :else "BOOLEAN")) | ||
|
|
||
| (defn- boolean-value-true |
There was a problem hiding this comment.
Can you clarify when this method returns true?
I feel it should be renamed for more ease of understanding.
| (key? column clustering-key) "CLUSTERING" | ||
| :else nil)) | ||
|
|
||
| (defn- get-secondary-indexed [rdb-engine column secondary-index] |
There was a problem hiding this comment.
If it returns true/false, should it be like is-secondary-indexed?
| "BOOLEAN" "BIT" | ||
| "BLOB" "VARBINARY"}}) | ||
|
|
||
| (def ^:private data-type-mapping-for-key |
Co-authored-by: Hiroyuki Yamada <[email protected]> Co-authored-by: Yuji Ito <[email protected]>
yito88
reviewed
Feb 11, 2021
| {:keys [boolean-value-fn] :as opts} | ||
| column data-type ordinal-position] | ||
| (let [key-type (get-key-type column partition-key clustering-key) | ||
| key-order (if (key? column clustering-key) "ASC" nil) |
There was a problem hiding this comment.
How about this? The corresponding if can be removed?
Suggested change
| key-order (if (key? column clustering-key) "ASC" nil) | |
| key-order (if (key? column clustering-key) "'ASC'" "NULL") |
|
It seems like the CI failure was not related to this change. I will rerun the CI. |
…e mapping for key
Co-authored-by: Yuji Ito <[email protected]>
feeblefakie
previously approved these changes
Feb 18, 2021
Co-authored-by: Hiroyuki Yamada <[email protected]>
Co-authored-by: Hiroyuki Yamada <[email protected]>
brfrn169
changed the title
feeblefakie
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://scalar-labs.atlassian.net/browse/DLT-7846
As I'm new to Clojure, this code might not be good. Please review it from the Clojure perspective as well 😄