-
Notifications
You must be signed in to change notification settings - Fork 310
Allow use of table options not registered in TableOption
and extend known TableOptions
#1586
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
Allow use of table options not registered in TableOption
and extend known TableOptions
#1586
Conversation
See spring-projects#1584 - default_time_to_live - cdc - speculative_retry - memtable_flush_period_in_ms - crc_check_chance - min_index_interval - max_index_interval - read_repair Signed-off-by: seungh0 <will.seungho@gmail.com>
b8f32ec
to
0444035
Compare
ab2ded1
to
e2d9c5a
Compare
return; | ||
} | ||
createTableSpecification.with(key, value, false, false); | ||
} |
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 the current situation, if users manually apply escaping and quoting, it could result in double escaping/quoting if the option is later added to TableOption in a future version.
So I considered two options:
- Apply escaping and quoting only when the option is not part of TableOption and the value is a string — as implemented in the PR.
- Enhance CqlStringUtils to detect whether the value is already escaped or quoted, and skip escaping if so.
I chose option 1, considering the potential side effects.
What do you think is the better approach?
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.
Providing values as strings in a map is convenient and doing so might cause a moment of surprise when an option (e.g. bloom_filter_fp_chance
) was not defined in TableOptions
and the value of 0.3
gets quoted to 0.3
. However, Cassandra seems not to care whether a value is quoted. Let's keep only the quoting and expect escaping by the caller.
Having the framework doing something that you cannot disable causes more inconvenience than doing less in the framework and allowing the caller to update their inputs.
see spring-projects#1584 Signed-off-by: seungh0 <will.seungho@gmail.com>
e2d9c5a
to
58f1595
Compare
TableOption
(+ Add support for additional table options)
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.
Thanks for the decent pull request. I'm going to merge this one for the next release.
TableOption
(+ Add support for additional table options)TableOption
and extend known TableOptions
Thank you for your contribution. That's merged and polished now. |
Allow use of table options not registered in TableOption
Add support for additional table options
Since I thought there might be different opinions on this commit,
I decided to split it into two separate commits for now.
Closes #1584