-
Notifications
You must be signed in to change notification settings - Fork 24
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
HPCC4J-650 DFSClient should retain integer subtypes #764
base: candidate-9.2.x
Are you sure you want to change the base?
Conversation
Jira Issue: https://hpccsystems.atlassian.net/browse/HPCC4J-650 Jirabot Action Result: |
@rpastrana Note: I am still working on correcting index record definition translation structure. |
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.
@jpmcmu looks good. but there's a couple of minor issues. Once resolved please squash
@@ -743,6 +760,10 @@ private static int getJsonTypeDefinition(FieldDef field, HashMap<Integer, Intege | |||
{ | |||
char delim = 0x0001; | |||
childJson.put("xpath", childField.getFieldName() + delim + "Row"); | |||
} else if (childField.getFieldType() == FieldType.SET) |
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.
minor format issue, missing new line
@@ -743,6 +760,10 @@ private static int getJsonTypeDefinition(FieldDef field, HashMap<Integer, Intege | |||
{ | |||
char delim = 0x0001; | |||
childJson.put("xpath", childField.getFieldName() + delim + "Row"); | |||
} else if (childField.getFieldType() == FieldType.SET) | |||
{ | |||
char delim = 0x0001; |
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 feels like this variable deserves to be a static final with an explicit self documenting name
- Modified record translation code to maintain integer subtype - Fixed record translation test case - Added index record translation to test case Signed-off-by: James McMullan [email protected]
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.
@jpmcmu just a couple of comments
this.defs = new FieldDef[rhs.defs.length]; | ||
for (int i = 0; i < rhs.defs.length; i++) | ||
{ | ||
this.defs[i] = new FieldDef(rhs.defs[i]); |
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.
why are we creating new FielDefs here, can we just copy rhs.defs[i] directly?
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.
Good question, I changed this to construct new FieldDefs here because I am going to potentially modify the SrcType later for KEYED_INTS. So I cannot share child FieldDefs any longer, as modifying the projected record definition would modify the original record definitions fields as well.
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.
Good question, I changed this to construct new FieldDefs here because I am going to potentially modify the SrcType later for KEYED_INTS. So I cannot share child FieldDefs any longer, as modifying the projected record definition would modify the original record definitions fields as well.
Fair enough, are there any situations where we could avoid creating the new FieldDef? If so, is it worthwhile attempting to detect the condition in order to avoid the ctr call?
* @return true when biased | ||
* @return true when biased | ||
* | ||
* @deprecated |
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.
let's add a comment indicating which method to use instead of this deprecated method
|
||
int childTypeHash = getJsonTypeDefinition(nonKeyedField, typeDefinitionMap, typeDefinitions); | ||
int childTypeIndex = typeDefinitionMap.get(childTypeHash); | ||
String childTypeName = "ty" + (childTypeIndex + 1); |
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's difficult to rationalize the significance of this literal "ty" let's externalize into a self documenting var
Signed-off-by: James McMullan [email protected]
Type of change:
Checklist:
Testing: