-
Notifications
You must be signed in to change notification settings - Fork 304
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-30776 ECL mods to support pluggable file format syntax *** WIP *** #18107
HPCC-30776 ECL mods to support pluggable file format syntax *** WIP *** #18107
Conversation
https://track.hpccsystems.com/browse/HPCC-30776 |
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're getting to grips with the structure. I would also add a new operator no_filetype, create that in setPluggableModeExpr
e.g.
targetAttr.setExpr(createValue(no_filetype, makeNullType(), createAttribute(fileFormat), *options));
Then search for no_csv in the sources in ecl/hql and add corresponding entries. You could also do the same for ecl/hqlcpp, but I would probably leave that to a separate PR, which meant the new syntax generated the generic disk read activity.
@@ -10529,6 +10529,75 @@ mode | |||
$$.setExpr(createValue(no_json, makeNullType(), args)); | |||
} | |||
| pipe | |||
| TYPE '(' FLAT ')' |
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.
Is it worth special casing these? I was expecting these to all generate the new format, and by implication be a way of explicitly using the new generic disk read code.
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 am taking this change very slowly while I learn the vast array of helpers and structs. As of this writing, I was focused solely on changing the syntax and making sure it worked. Next step will be changing the underlying code and how it is called.
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.
Also: A plugin-style filetype will process options differently than flex. This is definitely not a problem with a new filetype such as Parquet, but may cause backwards-compatibility issues with an existing filetype (errors may be caught at different times and/or reported differently). Is that a concern, given that this is introducing a new syntax?
ecl/hql/hqlgram2.cpp
Outdated
// TODO: Look for a plugin filetype of name fileFormatStr | ||
DBGLOG("HqlGram::setPluggableModeExpr processing file type %s", fileFormatStr.str()); | ||
|
||
// Following is a placeholder to make the parser happy -- note the hardwiring |
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.
whitespace: tabs being used for indentation rather than spaces.
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.
Fixed.
ecl/hql/hqlgram2.cpp
Outdated
IAtom * fileFormat = lower(mode.getId()); | ||
StringBuffer fileFormatStr(fileFormat->queryStr()); | ||
|
||
// TODO: Look for a plugin filetype of name fileFormatStr |
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.
Questionable whether it needs to check for a type at this point. Advantages and disadvantages...
8c393d8
to
ea1fb6c
Compare
3dbb5fb
to
b5cd3f3
Compare
b5cd3f3
to
0443f49
Compare
ad3d760
to
c4296cb
Compare
c4296cb
to
9a2c32d
Compare
Closing while continuing work. Will open a new draft later. |
Type of change:
Checklist:
Smoketest:
Testing: