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
HPCC-19070 Add support to base classes for substring matches on fields. #10861
Conversation
https://track.hpccsystems.com/browse/HPCC-19070 |
case type_data: | ||
case type_string: | ||
//Special case if source and destination types are identical to avoid cloning strings | ||
if ((destType.fieldType & sameTypeMask) == (sourceType.fieldType & sameTypeMask)) |
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.
This function moved - but this particular part of the code is new - to avoid cloning strings when resizing them. It will also potentially optimize some cases of projecting fields whether the change is the length that are not already special cased.
@richardkchapman please review. |
system/jlib/jbuff.hpp
Outdated
@@ -189,6 +188,8 @@ class jlib_decl MemoryBuffer | |||
inline void Release() const { delete this; } // for consistency even though not link counted | |||
|
|||
inline void * bufferBase() const { return buffer; } | |||
inline const char * toByteArray() const { return curLen ? buffer : NULL; } |
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.
use nullptr?
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.
code moved rather than new, but I will change it.
return values->matches(ptr + sizeof(size32_t)); | ||
|
||
//Clone and expand the string to the expected length | ||
byte * temp = (byte *)alloca(maxTempLength); |
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.
What happens when people use STRING1000000 ... Do they get what they deserve?
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.
It would only by triggered for
variablesizefieldstring[1..1000000] = 'x'
in which case, yes they probably get what they deserve! I will add an assert that the substring length is < 1000.
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.
Looks like you added a check that less than 256 ...
} | ||
|
||
return new VariableSubStringFieldFilter(fieldId, type, subType, values); | ||
} | ||
} | ||
|
||
UNIMPLEMENTED_X("Unknown Field Filter"); |
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.
Unimplemented? Or unexpected?
@ghalliday Some minor comments - looks good |
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
@richardkchapman see changes. |
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
Signed-off-by: Gavin Halliday gavin.halliday@lexisnexis.com
Type of change:
Checklist:
Testing: