CQL Heuristics Calculator - #1620
Conversation
…sion-replacement-v2
- Small refactorings to follow dev guidelines - Refactor parsing of Duration literals and reorganise packages - Unify distance calculation for strings - Refactor operation evaluator - Add javadoc for calculator and CassandraRow abstraction
- More refactorings in parser utils - Add missing docs in parser utils
…ent-v2 More CqlSession Replacements
- Use pattern for standard ISO - Improve CqlDurationLiteralParser and create unit tests class - Replace reflection with use of TemporalAmount
| } else if (op instanceof ContainsKeyOperation<?>) { | ||
| return evaluateContainsKey((ContainsKeyOperation<?>) op, candidateRow); | ||
| } else { | ||
| return FALSE_TRUTHNESS; |
There was a problem hiding this comment.
throw an exception for not supported evaluations
| private Truthness evaluateContains(ContainsOperation<?> op, CassandraRow candidateRow) { | ||
| Object rawCol = candidateRow.getValue(op.getColumnName()); | ||
| if (rawCol == null) { | ||
| return FALSE_TRUTHNESS; |
There was a problem hiding this comment.
this means that the column value for that column is null, or that the column is not defined?
There was a problem hiding this comment.
It means the column is not defined for that specific row! I'll modify the code so that it's not ambiguous
There was a problem hiding this comment.
what if a null value is stored in the candidateRow, how can you disambiguate?
| Object rawCol = candidateRow.getValue(op.getColumnName()); | ||
| if (rawCol == null) { | ||
| return FALSE_TRUTHNESS; | ||
| } else if (!(rawCol instanceof Map<?, ?>)) { |
There was a problem hiding this comment.
could a contain key be evaluated on a column that is not a map?
There was a problem hiding this comment.
According to the CQL documentation, that's not possible
There was a problem hiding this comment.
I'll add an exception for this scenario!
|
|
||
| private Truthness evaluateContainsKey(ContainsKeyOperation<?> op, CassandraRow candidateRow) { | ||
| Object rawCol = candidateRow.getValue(op.getColumnName()); | ||
| if (rawCol == null) { |
There was a problem hiding this comment.
again, is this a valid value for candidateRow.getValue(String) ?
There was a problem hiding this comment.
If the candidate row does not have that column defined, it could be possible
| Object rowValue = candidateRow.getValue(op.getColumnName()); | ||
| Object queryValue = op.getValue(); | ||
|
|
||
| if (rowValue == null && queryValue == null) { |
There was a problem hiding this comment.
Is NULL literal valid in CQL? I think it is not valid. Therefore, queryValue cannot have null as value, right?
There was a problem hiding this comment.
You're right, it's not. I'll change the code so that only rowValue is considered!
| CqlDurationLiteral literalDurationValue = CqlDurationLiteralParser.parse((String) literalValue); | ||
|
|
||
| return TruthnessUtils.buildAndAggregationTruthness( | ||
| TruthnessUtils.getEqualityTruthness(months, literalDurationValue.months), |
There was a problem hiding this comment.
define a variable for each getEqualityTruthness to improve readability
|
|
||
| private Truthness evaluateEquals(Object a, Object b) { | ||
| if (a == null && b == null) { | ||
| return FALSE_TRUTHNESS; |
There was a problem hiding this comment.
again, check semantics of NULL==NULL
| } else if (a == null || b == null) { | ||
| return FALSE_TRUTHNESS_BETTER; | ||
| } else { | ||
| Truthness raw = compareByType(a, b, ComparisonType.EQUALS); |
There was a problem hiding this comment.
It means it's the truthness instance before being scaled! I'll change the name
| if (t.isEmpty()) { | ||
| throw new IllegalArgumentException("Empty duration literal"); | ||
| } else { | ||
| boolean isNegative = t.startsWith("-"); |
There was a problem hiding this comment.
replace "-" with constant
| String upper = unsigned.toUpperCase(); | ||
| if (upper.endsWith(ISO_WEEK_SUFFIX)) { | ||
| return parseIso8601WeekPattern(upper); | ||
| } else if (upper.contains("-")) { |
| private Truthness evaluateContains(ContainsOperation<?> op, CassandraRow candidateRow) { | ||
| Object rawCol = candidateRow.getValue(op.getColumnName()); | ||
| if (rawCol == null) { | ||
| return FALSE_TRUTHNESS; |
There was a problem hiding this comment.
what if a null value is stored in the candidateRow, how can you disambiguate?
| public class CassandraHeuristicsCalculator { | ||
|
|
||
| public static final double C = DistanceHelper.H_NOT_NULL; | ||
| public static final double C_BETTER = 0.15; |
There was a problem hiding this comment.
either this C_BETTER should be computer from H_NOT_NULL (eg H_NOT_NULL + 0.05), or should be added directly to DistanceHelper with a more general name. otherwise, what if in the future we refactor to have H_NOT_NULL=0.2 for example?
There was a problem hiding this comment.
I added it to DistanceHelper so that the SQL calculator can use it directly from there.
I also did this for TRUE_TRUTHNESS, FALSE_TRUTHNESS , and FALSE_TRUTHNESS_BETTER, and added them to TruthnessUtils.
| // In CQL, a stored NULL and an absent/never-set column are the same thing at read time | ||
| // (writing NULL creates a tombstone, identical to never having written the column), so | ||
| // none of the null checks below (here and in evaluateContains, evaluateContainsKey and | ||
| // evaluateComparison) can tell these two cases apart. |
There was a problem hiding this comment.
for functions and fields, use /** */ so you get JavaDoc out of it
| * optional, mirroring the driver's own grammar, so bare {@code P} or {@code PT} are valid | ||
| * (zero-length) durations. | ||
| */ | ||
| private static final Pattern ISO8601_PATTERN = Pattern.compile( |
There was a problem hiding this comment.
a lot of the code here seems generic about date handling, and not specific to Cassandra, isn't it? if so, should refactor in some common re-usable utility functions not bound to Cassandra (as it could be re-used in other contexts, if needed)
There was a problem hiding this comment.
I tried moving some of the logic to a new class IsoDurationParser, let me know what you think!
- Abstract parsing of ISO duration format - Turn comments into javadoc - Unify distance and truthness related constants
Added CassandraHeuristicsCalculator/CassandraOperationEvaluator to compute a distance heuristic for CQL WHERE clauses
Distance calculation for each operator/data type follows the spec from this document