In PR #107
Conversation
… confirmation on null behavior and also I wonder why integer field is sufficient for string
| + std::string(tuix::EnumNameFieldUnion(item->value_type()))); | ||
| } | ||
| //TODO why integer field passing the string test | ||
| if (static_cast<const tuix::IntegerField *>(item->value())->value() == static_cast<const tuix::IntegerField *>(left->value())->value()){ |
There was a problem hiding this comment.
I think you might be forcibly casting here from string to integer, and the test passes because the integer values are the same.
There was a problem hiding this comment.
Used the dynamically comparison operator to fix this problem.
|
|
||
| } | ||
|
|
||
| case tuix::ExprUnion_Concat: |
There was a problem hiding this comment.
Can you split this into a separate PR?
There was a problem hiding this comment.
sorry this is still in the same PR but both are somewhat complete now. I will look into splitting PR in the future.
| //TODO add null check | ||
|
|
||
| return tuix::CreateField( | ||
| builder, | ||
| tuix::FieldUnion_StringField, | ||
| tuix::CreateStringFieldDirect( | ||
| builder, &result, static_cast<uint32_t>(total)).Union(), | ||
| false); | ||
| /* | ||
| auto array_field = static_cast<const tuix::ArrayField *>(value->value()); | ||
| std::string str = to_string(array_field); | ||
| std::vector<uint8_t> str_vec(str.begin(), str.end()); | ||
| return tuix::CreateField( | ||
| builder, | ||
| tuix::FieldUnion_StringField, | ||
| tuix::CreateStringFieldDirect(builder, &str_vec, str_vec.size()).Union(), | ||
| result_is_null); | ||
| */ |
There was a problem hiding this comment.
Usually PRs don't include unfinished code or commented out code like this.
| case (Concat(child), childrenOffsets) => | ||
| tuix.Expr.createExpr( | ||
| builder, | ||
| tuix.ExprUnion.Concat, | ||
| tuix.Concat.createConcat( | ||
| builder, tuix.Concat.createChildrenVector(builder, childrenOffsets.toArray))) | ||
|
|
There was a problem hiding this comment.
Please put this in a separate PR.
There was a problem hiding this comment.
sorry this is still in the same PR but both are somewhat complete now. I will look into splitting PR in the future.
| bool result = false; | ||
|
|
||
| if (num_children < 2){ | ||
| throw std::runtime_error(std::string("In can't operate with fewer than 2 args, currently we have") |
There was a problem hiding this comment.
Can this error be a bit more descriptive? What are the arguments that the In expression is expecting?
| } | ||
|
|
||
| auto left_offset = eval_helper(row, (*c->children())[0]); | ||
| const tuix::Field *left = flatbuffers::GetTemporaryPointer(builder, left_offset); |
There was a problem hiding this comment.
We need to be able to handle NULL values, an example here.
| size_t num_children = c->children()->size(); | ||
| bool result = false; | ||
|
|
||
| if (num_children < 2){ |
There was a problem hiding this comment.
Can you verify what kind of types IN handles? What if the user passes in a CalendarType?
There was a problem hiding this comment.
Used a dynamic typed comparison method that should work with different types
| df.filter($"word".contains(lit("1"))).collect | ||
| } | ||
|
|
||
| testAgainstSpark("concatwithstring") { securityLevel => |
There was a problem hiding this comment.
Can you put spaces in the test name? Otherwise it's hard to read.
| df.select(concat(col("str"),lit(","),col("x"))).collect | ||
| } | ||
|
|
||
| testAgainstSpark("concatwithotherdatatype") { securityLevel => |
| val df = makeDF(ids, securityLevel, "x", "y", "id") | ||
| val c = $"id" isin ($"x", $"y") | ||
| val result = df.filter(c) | ||
| //result.explain(true) |
| val df2 = makeDF(ids2, securityLevel, "x", "y", "id") | ||
| val c2 = $"id" isin (1 ,2, 4, 5, 6) | ||
| val result = df2.filter(c2) | ||
| //result.explain(true) |
| val df3 = makeDF(ids3, securityLevel, "x", "y", "id") | ||
| val c3 = $"id" isin ("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ,"b", "c", "d", "e") | ||
| val result = df3.filter(c3) | ||
| //result.explain(true) |
| val df4 = makeDF(ids4, securityLevel, "x", "y", "id") | ||
| val c4 = $"id" isin (null.asInstanceOf[Int]) | ||
| val result = df4.filter(c4) | ||
| //result.explain(true) |
| + std::string(tuix::EnumNameFieldUnion(str->value_type())) | ||
| + std::string(". You do not need to provide the data as string but the data should be serialized into string before sent to concat")); | ||
| } | ||
| if (!str->is_null()){ |
There was a problem hiding this comment.
Is the final result NULL if any one value is NULL? If so, why do you need to go through concat with the non-NULL strings?
There was a problem hiding this comment.
The final result is not NULL if one value is null. It just skips over the null value. Modified a test to reflect this
| eval_binary_comparison<tuix::EqualTo, std::equal_to>( | ||
| builder, | ||
| flatbuffers::GetTemporaryPointer<tuix::Field>(builder, left_offset), | ||
| flatbuffers::GetTemporaryPointer<tuix::Field>(builder, right_offset))) | ||
| ->value())->value(); |
There was a problem hiding this comment.
This doesn't work for every type, should you check for the supported type here?
There was a problem hiding this comment.
The eval_binary_comparison does the type checking and throws an type error if incompatible data type is passed in which is sufficient for type checking?
finishing in expression. adding more tests and null support. need confirmation on null behavior and also I wonder why integer field is sufficient for string comparison. will bring this up at the meeting.