HPCC-31496 Allow field translation that only removes fields - and does not add blank values #18496

jackdelv · 2024-04-05T12:10:11Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2024-04-05T12:11:07Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31496

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

jackdelv · 2024-04-05T12:12:13Z

@ghalliday Is this the behaviour that you were imagining? Is this the best place to add the PayloadRemoveOnly logic?

richardkchapman

There are other references to RecordTranslationMode::Payload that you might want to change, if the intention was to change the default.

But I don't think as coded this actually works as intended.

richardkchapman · 2024-04-10T08:31:43Z

common/thorhelper/thorcommon.cpp

+            if (publishedCrc && expectedCrc && (publishedCrc != expectedCrc))
+            {
+                DBGLOG("Overriding stored record layout reading file %s", tracing);
+                if (expectedFormat->queryRecordAccessor(true).getNumFields() > publishedFormat->queryRecordAccessor(true).getNumFields())


I'm not sure if this is supposed to check that fields were only removed and not added, but how would it handle a case where fields were both added and removed (but more were removed than added)?

I fixed the check to use match_none. Now it strictly checks for added fields.

richardkchapman · 2024-04-10T08:32:55Z

roxie/ccd/ccdfile.cpp

+                        if (formatCrcs.item(idx) && expectedFormatCrc && (formatCrcs.item(idx) != expectedFormatCrc))
+                        {
+                            DBGLOG("Overriding stored record layout reading file %s", subname);
+                            if (expected->queryRecordAccessor(true).getNumFields() > diskTypeInfo.item(idx)->queryRecordAccessor(true).getNumFields())


Same comment as before.

richardkchapman · 2024-04-10T08:40:09Z

rtl/eclrtl/rtldynfield.hpp

@@ -110,7 +110,8 @@ enum class RecordTranslationMode : byte
    Payload = 2,    // Translate all fields in datasets, and only payload fields in indexes
    AlwaysDisk = 3, // Always translate - even if wouldn't normally (e.g. csv/xml source read as binary), or crcs happen to match
    AlwaysECL = 4,  // Ignore the published format - can make sense to force no translation e.g. when field names have changed
-    Unspecified = 5
+    Unspecified = 5,
+    PayloadRemoveOnly = 6 // Allow fields to be removed from the incoming dataset, but not allow fields to be missing


I would have made Unspecified be 6... these values are not persisted anywhere AFAIK

Changed Unspecified to 6.

ghalliday · 2024-04-10T12:58:07Z

@jackdelv following on from Richard's comments. You need to add a method to IDynamicTransform e.g. hasNewFields(). Take a look at match_none in FieldMatchType which should be set if that is the case.

jackdelv · 2024-04-10T19:21:24Z

@richardkchapman @ghalliday I addressed your comments and made appropriate changes. However, I am uncertain how to verify that I have covered all the cases where hasNewFields needs to be called. I looked in the repo for uses of "RecordTranslationMode::Payload", "payload", "needsTranslate". Should there be a check for added fields everywhere needsTranslate is used?

ghalliday

This looks like it is close to being ready

needs to be a non-draft PR
I think it is missing a check in slavmain.cpp(~355)
Missing some places in hthorkey.cpp (search for canTranslate())

I am not quite sure what version to target. Probably 9.6.x. An alternative, is to target the change to 9.4.x, by the change in defaults to 9.6..x.
@jakesmith thoughts?

ghalliday · 2024-04-11T09:29:40Z

helm/hpcc/values.schema.json

-          "default": "payload",
-          "enum": ["false", "true", "payload"],
+          "default": "PayloadRemoveOnly",
+          "enum": ["false", "true", "PayloadRemoveOnly"],


payload still needs to be in the list as a valid option. It would be better to have consistent casing with the other options - a leading lower case letter.

ghalliday · 2024-04-11T09:29:56Z

initfiles/componentfiles/configschema/xsd/eclagent.xsd

                                  hpcc:tooltip="Enables translation (where possible) of mismatched index layouts on-the-fly. Specify 'payload' to attempt to translate payload fields only">
                        <xs:simpleType>
                            <xs:restriction base="xs:string">
                                <xs:enumeration value="false" hpcc:displayName="False" hpcc:desciption=""/>
                                <xs:enumeration value="true" hpcc:displayName="true" hpcc:description=""/>
-                                <xs:enumeration value="payload" hpcc:displayName="Payload" hpcc:description=""/>


Enumeration needs to still include payload. Same for all other instances.

ghalliday · 2024-04-11T09:32:35Z

rtl/eclrtl/rtldynfield.cpp

@@ -60,6 +62,7 @@ extern ECLRTL_API const char *getTranslationModeText(RecordTranslationMode val)
    case RecordTranslationMode::AlwaysDisk: return "alwaysDisk";
    case RecordTranslationMode::AlwaysECL: return "alwaysECL";
    case RecordTranslationMode::Payload: return "payload";
+    case RecordTranslationMode::PayloadRemoveOnly: return "PayloadRemoveOnly";


leading lower case for consistency

Issue has been addressed

jackdelv · 2024-04-11T13:54:33Z

@ghalliday I add payload back as an option and changed the casing to be more consistent. I also added checks in slavmain.cpp and hthorkey.cpp. Back to you.

jakesmith

@jackdelv - added a couple of minor comments.

jakesmith · 2024-04-15T09:13:26Z

common/thorhelper/thorread.cpp

@@ -176,6 +176,9 @@ void DiskReadMapping::ensureTranslators() const
        if (!translator->canTranslate())
            throw MakeStringException(0, "Untranslatable record layout mismatch detected for file %s", filename);

+        if (mode == RecordTranslationMode::PayloadRemoveOnly && translator->hasNewFields())
+                throw MakeStringException(0, "Untranslatable record layout mismatch detected for file %s. Expected fields were not found in source.", filename);


formatting: indentation off.

jakesmith · 2024-04-15T09:13:46Z

ecl/hthor/hthorkey.cpp

@@ -700,6 +700,8 @@ const IDynamicTransform * CHThorIndexReadActivityBase::getLayoutTranslator(IDist
    Owned<const IDynamicTransform> payloadTranslator =  createRecordTranslator(projectedFormat->queryRecordAccessor(true), actualFormat->queryRecordAccessor(true));
    if (!payloadTranslator->canTranslate())
        throw MakeStringException(0, "Untranslatable key layout mismatch reading index %s", f->queryLogicalName());
+    if (getLayoutTranslationMode() == RecordTranslationMode::PayloadRemoveOnly && payloadTranslator->hasNewFields())
+                throw MakeStringException(0, "Untranslatable record layout mismatch detected for file %s. Expected fields were not found in source.", f->queryLogicalName());


formatting: indentation off.

jakesmith · 2024-04-15T09:16:57Z

common/thorhelper/thorcommon.cpp

@@ -2148,6 +2148,9 @@ static bool getTranslators(Owned<const IDynamicTransform> &translator, Owned<con
            if (!translator->canTranslate())
                throw MakeStringException(0, "Untranslatable record layout mismatch detected for file %s", tracing);

+            if (mode == RecordTranslationMode::PayloadRemoveOnly && translator->hasNewFields())
+                throw MakeStringException(0, "Untranslatable record layout mismatch detected for file %s. Expected fields were not found in source.", tracing);


minor: It is translatable, but mode prohibits it. Consider different message here (and similarly elsewhere).

Changed message.

jakesmith · 2024-04-17T09:10:17Z

initfiles/etc/DIR_NAME/environment.xml.in

@@ -68,7 +68,7 @@
                enableKeyDiff="true"
                enableSysLog="true"
                fastLaneQueue="true"
-                fieldTranslationEnabled="payload"
+                fieldTranslationEnabled="payloadRemoveOnly"


This should be consistent with testing/regress/environment.xml.in.

The changes between initfiles/etc/DIR_NAME/environment.xml.in and testing/regress/environment.xml.in look consistent to me. I'm probably misunderstanding what you mean.

agreed, I missed the testing/regress change.

jakesmith · 2024-04-17T09:12:48Z

I am not quite sure what version to target. Probably 9.6.x. An alternative, is to target the change to 9.4.x, by the change in defaults to 9.6..x.
@jakesmith thoughts?

I think it's safe enough to go into 9.4 if we leave the default as 'payload'. And if many are going to stick with 9.4 for a while, and want this feature, they'll have it there. So I'd vote for this targeting 9.4

ghalliday

Looks good to me

ghalliday · 2024-04-22T15:35:46Z

@jackdelv the regression tests will need to be updated to set the option in the work unit to override the default

jackdelv · 2024-04-22T17:58:04Z

@ghalliday There are still some regression tests failing like sqtrans.ecl that hadn't previously set a value for the translationMode. Is overriding the default in the ecl files correct, or did you mean something different?

ghalliday · 2024-04-23T15:52:31Z

@jackdelv yes, please add a similar #option into those files to force the old payload option on.

jackdelv · 2024-04-23T17:43:06Z

@ghalliday I believe all the necessary regression tests have been fixed.

jakesmith

@jackdelv - looks good.

ghalliday

Looks good. Just re-running k8s tests.
Also debating whether this should go in 9.6.x. It is a bit late - because it will potentially cause existing queries to fail, which shouldn't happen in a point release. However, without it there is no safe way of enabiling field translation if you do not want missing fields to be allowed.

Possibly one way forward would be to merge it to 9.6.x with payload as the default, and then change the default for master/9.8.x.

ghalliday · 2024-04-26T09:23:33Z

Please squash, and lets talk about the target build.

…s not add blank values

jackdelv · 2024-04-29T11:53:28Z

@ghalliday Squashed.

ghalliday

@jackdelv please can you split this PR in two.
One set of changes that targets 9.6.x which adds the code to process everything, but does not change the default. Then a second PR targeting master that changes the default.

jackdelv requested a review from ghalliday April 5, 2024 12:12

richardkchapman previously requested changes Apr 10, 2024

View reviewed changes

jackdelv requested a review from richardkchapman April 10, 2024 19:21

ghalliday reviewed Apr 11, 2024

View reviewed changes

ghalliday requested review from jakesmith and removed request for richardkchapman April 11, 2024 09:38

ghalliday requested review from richardkchapman and removed request for richardkchapman April 11, 2024 09:39

jackdelv marked this pull request as ready for review April 11, 2024 13:52

jackdelv requested a review from ghalliday April 11, 2024 13:54

jakesmith reviewed Apr 15, 2024

View reviewed changes

jackdelv requested a review from jakesmith April 15, 2024 12:41

jakesmith reviewed Apr 17, 2024

View reviewed changes

ghalliday approved these changes Apr 22, 2024

View reviewed changes

ghalliday requested review from jakesmith and richardkchapman April 22, 2024 15:35

jackdelv requested a review from ghalliday April 23, 2024 14:20

jakesmith approved these changes Apr 26, 2024

View reviewed changes

ghalliday approved these changes Apr 26, 2024

View reviewed changes

HPCC-31496 Allow field translation that only removes fields - and doe…

7ee26ab

…s not add blank values

jackdelv force-pushed the HPCC-31496 branch from 354f367 to 7ee26ab Compare April 29, 2024 11:51

jackdelv requested a review from ghalliday April 29, 2024 12:03

ghalliday reviewed May 1, 2024

View reviewed changes

richardkchapman approved these changes May 1, 2024

View reviewed changes

jackdelv closed this May 1, 2024

HPCC-31496 Allow field translation that only removes fields - and does not add blank values #18496

HPCC-31496 Allow field translation that only removes fields - and does not add blank values #18496

Conversation

jackdelv commented Apr 5, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Apr 5, 2024

jackdelv commented Apr 5, 2024

richardkchapman left a comment

Choose a reason for hiding this comment

richardkchapman Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Apr 10, 2024

jackdelv commented Apr 10, 2024 • edited Loading

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdelv commented Apr 11, 2024

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

jackdelv Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Apr 17, 2024

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday commented Apr 22, 2024

jackdelv commented Apr 22, 2024

ghalliday commented Apr 23, 2024

jackdelv commented Apr 23, 2024

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday commented Apr 26, 2024

jackdelv commented Apr 29, 2024

ghalliday left a comment

Choose a reason for hiding this comment

jackdelv commented Apr 5, 2024 •

edited

Loading

richardkchapman Apr 10, 2024 •

edited

Loading

jackdelv commented Apr 10, 2024 •

edited

Loading

jakesmith Apr 17, 2024 •

edited

Loading

jackdelv Apr 17, 2024 •

edited

Loading