Add javascript and regex parsers #36
Conversation
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
pls use newer license header
| parsed | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
can you either add another UT or update this so that field names are explicitly specified and resulting map has something else and not "column_1"?
|
@himanshug @gianm Addressed all comments |
fjy
force-pushed
the
master
branch
3 times, most recently
from
fe706a4
to
8d6407c
Compare
| try { | ||
| final Object compiled = fn.apply(input); | ||
| if (!(compiled instanceof Map)) { | ||
| throw new ParseException("JavaScript parsed value must in {key: value} format!"); |
|
👍 |
| return Utils.zipMapPartial(fieldNames, Iterables.transform(values, valueFunction)); | ||
| } | ||
| catch (Exception e) { | ||
| throw new ParseException(e, "Unable to parse row [%s]", input); |
There was a problem hiding this comment.
probably not a big issue, but if matching fails then user would get nested ParseException, one from line 92 then that will get wrapped inside another here, should we catch specific exceptions instead of Exception ? or may be add another catch for ParseException that simple throws that?
There was a problem hiding this comment.
the two levels of parseexceptions will tell you exactly what is wrong and where it is wrong. I tested this
|
👍 |
| final ScriptableObject scope = context.initStandardObjects(); | ||
|
|
||
| final org.mozilla.javascript.Function fn = context.compileFunction(scope, function, "fn", 1, null); | ||
| Context.exit(); |
There was a problem hiding this comment.
This seems to use ThreadLocal variables, which is interesting.
There was a problem hiding this comment.
this is what we do for all the javascript aggregators, filters, etc.
There was a problem hiding this comment.
can we move the context.exit to finally, to make sure any error does not leaves the thread local set ?
|
@fjy is there a reason those interfaces cannot be implemented inside of the druid repository? I'd rather not add a dependency to rhino just for that, if possible. |
|
@xvrl we can submit another PR to move all these interfaces out of java-util |
|
Can you add unit tests for failure cases? |
|
What happens if the regex has an optional group? |
|
@drcrallen what kind of failure cases? |
|
@fjy failure for the matcher in the regex, failure for functions returning non-map things for the javascript come to mind immediately. |
|
@drcrallen can we do this in a later PR? file the issue for me |
|
|
||
| public RegexParser( | ||
| final String pattern, | ||
| final Optional<String> listDelimiter |
There was a problem hiding this comment.
how about making the delimiter a regex as well?
There was a problem hiding this comment.
Splitter.on(...) supports passing a pattern, that would make it a lot more powerful
| } | ||
|
|
||
| final Object res = fn.call(cx, scope, scope, new Object[]{input}); | ||
| return res != null ? Context.toObject(res, scope) : null; |
There was a problem hiding this comment.
You can use Context.jsToJava(res, Map.class) to avoid having to cast things later on I think
No description provided.