-
Notifications
You must be signed in to change notification settings - Fork 310
HPCC-35694 DFUQuery sort by compressed size #20885
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
base: candidate-10.2.x
Are you sure you want to change the base?
Changes from 3 commits
237b8fa
9b85d09
fe68908
342f4dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -14203,11 +14203,15 @@ static IPropertyTreeIterator *deserializeFileAttrIterator(MemoryBuffer& mb, unsi | |||||||
|
|
||||||||
| if (options.includeField(DFUQResultField::size)) | ||||||||
| { | ||||||||
| // JCSMORE - I am not sure what the point of this is, with or without it, a blank @size does not affect sort order | ||||||||
| // and EclWatch seems to use the @size (DFUQResultField::origsize) for size column anyway | ||||||||
| // See special handling in SerializeFileAttrOptions::readFields() to include origsize if size is requested | ||||||||
| // Size field is notionally the size on disk | ||||||||
| const char *propName = getDFUQResultFieldName(DFUQResultField::size); | ||||||||
| attr->setPropInt64(propName, attr->getPropInt64(getDFUQResultFieldName(DFUQResultField::origsize), -1));//Sort the files with empty size to front | ||||||||
| attr->setPropInt64(propName, isCompressed(*attr) ? attr->getPropInt64(getDFUQResultFieldName(DFUQResultField::compressedsize), -1) : attr->getPropInt64(getDFUQResultFieldName(DFUQResultField::origsize), -1)); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DFUQResultField::size is a special field that only this sets (@DFUSFsize) @GordonSmith - as far I could see last time I looked, Eclwatch (nor nothing else uses it).. I thought Eclwatch used "@SiZe" which is DFUQResultField::origsize .. ? (see comment in code here prior to this change). If so this code should be deleted (and probably DFUQResultField::size/@DFUSFsize, but that could be part of a separate cleanup PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To elaborate on what Jake's saying here as best as I can see, the The missing link is that the Currently it pulls The DFUQuery response would probably need to be modified with a new field for something like FileSize to hold https://github.com/hpcc-systems/HPCC-Platform/blob/master/esp/scm/ws_dfu_common.ecm |
||||||||
| } | ||||||||
GordonSmith marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| if (options.includeField(DFUQResultField::origsize)) | ||||||||
| { | ||||||||
| const char *propName = getDFUQResultFieldName(DFUQResultField::origsize); | ||||||||
| attr->setPropInt64(propName, attr->getPropInt64(getDFUQResultFieldName(DFUQResultField::origsize), -1)); | ||||||||
|
||||||||
| attr->setPropInt64(propName, attr->getPropInt64(getDFUQResultFieldName(DFUQResultField::origsize), -1)); | |
| if (attr->hasProp(propName)) | |
| attr->setPropInt64(propName, attr->getPropInt64(propName)); |
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.
I kind of agree I suppose, but not quite what copilot is suggesting. The code is trying to default this prop to -1 if not set, so it would be better to do:
if (!attr->hasProp(propName))
attr->setPropInt64(propName, -1);
But, re. prev comments that were on lines 14206-14208..
and comment at end of line on 14210 "//Sort the files with empty size to front"
Aren't blank entries already at front?
GordonSmith marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2683,6 +2683,9 @@ class DaliDFSIteratorTests : public CppUnit::TestFixture | |
| attr.setProp("@job", attrValue); | ||
| attr.setProp("@owner", attrValue); | ||
| attr.setProp("@workunit", attrValue); | ||
| attr.setPropBool("@rowCompressed", true); | ||
| attr.setPropInt64("@compressedSize", 9); | ||
| attr.setPropInt64("@size", 17); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2857,6 +2860,9 @@ class DaliDFSIteratorTests : public CppUnit::TestFixture | |
| CPPUNIT_ASSERT_MESSAGE("testGetLogicalFilesSorted: Missing cost attributes", costAttrsPresent); | ||
| bool dirAttrsPresent = attrs.hasProp("@directory"); | ||
| CPPUNIT_ASSERT_MESSAGE("testGetLogicalFilesSorted: directory attribute should NOT be present", !dirAttrsPresent); | ||
| CPPUNIT_ASSERT_MESSAGE("testGetLogicalFilesSorted: size field missing", attrs.hasProp("@DFUSFsize")); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why we have @DFUSFsize at the moment - what if anything uses it? |
||
| CPPUNIT_ASSERT_MESSAGE("testGetLogicalFilesSorted: compressed size field missing", attrs.hasProp("@compressedSize")); | ||
| CPPUNIT_ASSERT_MESSAGE("testGetLogicalFilesSorted: size should use compressed size", attrs.getPropInt64("@DFUSFsize", -1) == attrs.getPropInt64("@compressedSize", -1)); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.