Skip to content

Conversation

@gweidner
Copy link
Member

This patch updates parser component to allow scripts to run in cases where circular source references exist. Previously, a circular import (e.g., a script sources itself) would result in stack overflow error where
DMLParserWrapper.doParse and DMLSyntacticValidator.exitImportStatement infinitely loop during ParseTreeWalker.walk. Test scripts are included for both DML and PyDML to exercise various forms of circular dependencies.

@gweidner
Copy link
Member Author

The new code tracks imported scripts in a static variable and skips/defers building the corresponding DMLProgram for redundant/circular reference. A static variable was used to facilitate access in exitImportStatement across multiple DMLSyntacticValidator's which get instantiated during processing.
Note importing the same script under different namespace is allowed (namespace conflicts to be addressed in SYSTEMML-631).

All tests passed in OnDemand build 149.

@mboehm7
Copy link
Contributor

mboehm7 commented Apr 22, 2016

thanks @gweidner that looks already pretty good - just two comments.

  1. Thread-local state: could you please change the global state (static map) to thread-local state management in order to prevent side-effects between concurrent scripts parsing in a shared JVM process. The scenario is JMLC scoring (one prepared script per core) in a shared Spark executor process.
  2. Consistency function keys: Internally (independent of the language facet), we try to always use DMLProgram.constructFunctionKey() for building fully qualified function names. It would be good to reuse that in order to avoid surprises for devs expecting a certain schema.

@gweidner
Copy link
Member Author

Thank you @mboehm7 for your time and always helpful comments. I've incorporated ThreadLocal for the static map in CommonSyntacticValidator and constructFunctionKey for the map's keys in DML/PyDMLSyntacticValidator. I patterned ThreadLocal changes after similar ThreadLocal usage in hops Recompiler but please let me know if I've misinterpreted anything.

Prior to pushing this commit of 3 files I rebased and pushed this branch to merge in latest parser updates from master. PR build 287 in progress for commits containing ThreadLocal.

@mboehm7
Copy link
Contributor

mboehm7 commented Apr 22, 2016

LGTM - thanks @gweidner

@deroneriksson
Copy link
Member

Thank you @gweidner . I will merge.

@asfgit asfgit closed this in 691cbb8 Apr 23, 2016
@deroneriksson
Copy link
Member

Thanks Glenn for this great contribution! I'll close the JIRA since Apache JIRA is on 'lockdown' mode today.

@dusenberrymw
Copy link
Contributor

Thanks, @gweidner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants