PathExtraction initinal implementaion #1

raganhan · 2018-10-17T02:41:44Z

Path Extraction implementation that exposes the IonReader to
registered callbacks
Package setup, e.g. gradle, checkstyle, copyright

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* Path Extraction implementation that exposes the IonReader to registered callbacks * Package setup, e.g. gradle, checkstyle, copyright

tgregg

Very nice. Good tests. Some comments below.

tgregg · 2018-10-17T18:47:53Z

src/main/java/software/amazon/com/ionpathextraction/PathComponentParser.java

+ */
+class PathComponentParser {
+
+    private static final IonSystem ION_SYSTEM = IonSystemBuilder.standard().build();


I recommend replacing this with an IonReaderBuilder and an IonTextWriterBuilder; a full IonSystem is no longer required for constructing readers and writers.

good idea, will change it

tgregg · 2018-10-17T18:50:07Z

src/main/java/software/amazon/com/ionpathextraction/PathComponentParser.java

+ * language governing permissions and limitations under the License.
+ */
+
+package software.amazon.com.ionpathextraction;


The com is superfluous. For example, ion-java uses software.amazon.ion.

thanks for noticing that, I'll submit the change in namespace as a separate PR to avoid polluting this one, #2

tgregg · 2018-10-17T18:56:56Z

src/main/java/software/amazon/com/ionpathextraction/PathComponentParser.java

+
+    private boolean isWildcard(final IonReader reader) {
+        if (reader.stringValue().equals(Wildcard.TEXT)) {
+            for (final Iterator<String> iter = reader.iterateTypeAnnotations(); iter.hasNext(); ) {


For consistency with ion-c, this should only check to see if the first annotation is WILDCARD_ESCAPE_ANNOTATION. (Processing of local symbol tables serves as precedent for this--only structs whose first annotation is $ion_symbol_table are treated as local symbol tables.)

tgregg · 2018-10-17T19:09:12Z

src/main/java/software/amazon/com/ionpathextraction/PathExtractorBuilder.java

+    private static final boolean DEFAULT_MATCH_RELATIVE_PATHS = false;
+    private static final boolean DEFAULT_CASE_INSENSITIVE = false;
+    private final List<SearchPath> searchPaths = new ArrayList<>();
+    private final List<Function<IonReader, Integer>> callbacks = new ArrayList<>();


I do think there is value in giving the user the ability to pass through some context to the callback. The first adopter of the ion-c path extractor is using this feature. That would make the callback function a BiFunction<IonReader, Object, Integer>, and you'd probably have to add a class to hold the pair of the function and Object context.

Then again, maybe this can be achieved in Java with no loss of functionality if the author of the callback just refers to external variables containing any desired context. If that's the case, then we can keep it as-is, and we should change the developers' guide to make it clear that presence of user context in the callback is is left to the implementation based on what's easiest in the particular language.

In Java the user can use the lambda closure or a concrete Function implementation as the user context. The callback I use in the tests uses the lambda closure for this so it can accumulate all results in a IonList

Will add that as an example in the documentation

tgregg · 2018-10-17T19:16:14Z

src/main/java/software/amazon/com/ionpathextraction/PathComponentParser.java

+
+    private static final String WILDCARD_ESCAPE_ANNOTATION = "$ion_extractor_field";
+
+    List<PathComponent> parse(final String ionPathExpression) {


Is there a reason not to make all these methods static and not require an actual instance of PathComponentParser?

Not really.

I was thinking of having interfaces for all these classes to make it easier to swap implementations eventually but since PathComponentParser is not public this is not necessary. I should do that for PathExtractor at least though

Will change PathComponentParser and add an interface to PathExtractor

tgregg · 2018-10-17T21:09:49Z

src/main/java/software/amazon/com/ionpathextraction/PathExtractor.java

+        }
+
+        int getCurrentDepth() {
+            return stack.size() - 1;


It looks like this could cause problems when there are no search paths. Maybe we should do a quick return in match when that's the case?

Or we can require at least one search path, but currently ion-c and the documentation don't require that.

I'll keep parity with ion-c here as it can be useful to have a no-op PathExtractor, i.e. with zero search paths

I added some tests and it works with zero paths.
Take a look at PathExtractor#matchRecursive, even though currentDepth will be -1 it is not used by anything as there are no search paths, and thus no active paths skipping the for body where currentDepth is used

Agree that a short circuit is good here as it makes the code clearer and avoids unnecessary work so will add that.

tgregg · 2018-10-17T21:11:28Z

src/test/kotlin/software/amazon/com/ionpathextraction/PathExtractorTest.kt

+
+        assertEquals(ION.singleValue("[1,2,3]"), out)
+    }
+


Please add a test case for calling match with zero registered search paths.

And one for returning an integer from a matched callback that is greater than the reader's depth.

will do. I'm assuming that the callback return should not be higher than the reader current relative depth to account for matchRelativePaths

example

data {foo: {bar: 1}} search path: (bar) return 2 // max

edit: was supposed to say "should not be higher" instead of "should be higher"

Yes, correct.

tgregg · 2018-10-17T21:29:26Z

src/main/java/software/amazon/com/ionpathextraction/PathExtractor.java

+
+    private int invokeCallback(final IonReader reader, final SearchPath searchPath) {
+        int previousReaderDepth = reader.getDepth();
+        int stepOutTimes = callbacks.get(searchPath.getId()).apply(reader);


Fail if stepOutTimes is greater than the reader's depth (as ion-c does)?

It doesn't, will fix

tgregg · 2018-10-17T21:34:55Z

src/main/java/software/amazon/com/ionpathextraction/PathExtractorBuilder.java

+    private static final boolean DEFAULT_MATCH_RELATIVE_PATHS = false;
+    private static final boolean DEFAULT_CASE_INSENSITIVE = false;
+    private final List<SearchPath> searchPaths = new ArrayList<>();
+    private final List<Function<IonReader, Integer>> callbacks = new ArrayList<>();


Instead of having two parallel ArrayLists, how about modifying the SearchPath class to hold a List<PathComponent> and the callback Function (instead of an Integer id, which is only used as an index into the callbacks list).

will do, see my answers to your comments on SearchPath above for more context

tgregg · 2018-10-17T21:49:05Z

src/main/java/software/amazon/com/ionpathextraction/PathExtractor.java

+            "reader must be at depth zero, it was at:" + reader.getDepth());
+
+        // marks all search paths as active
+        tracker.reset(searchPaths);


If we wanted PathExtractor to be thread-safe (with the caveat that each thread would need to use a different IonReader), it looks like it would be as simple as instantiating a new Tracker on each invocation of match.

Correct. Trade off is more instantiation every match call. TBH I'm not sure how big that trade off is so erred on making it faster instead of thread safe as you can pool PathExtractors easily.

In any case should be to refactor so we can have both versions and let the user decide. Would wait on a use case before benchmarking both versions and/or providing both versions

Showing how you can use the lambda closure as ion-c user context

making the wildcard escape annotation valid only as the first annotation. Added a test to show that

PathExtractorImpl is now package private to make easier to add new implementations in the future if necessary

Already worked, but short circuiting makes it clearer and avoids unecessary work. Also adds missing tests for this use case

relative depth

and some other spelling and missing comments

* Moving callback to SearchPath * Simplifying partial match detection logic * Fixing needToStepIn logic * Adding tests for nested search paths

raganhan · 2018-10-18T17:37:58Z

Tried to address each comment in a separate commit to make it easier for the reviewer.

I also renamed PathExtractor to PathExtractorImpl and made PathExtractor an interface. I can undo this change if it makes it easier to review and re-apply before merging

tgregg · 2018-10-18T19:37:55Z

README.md

+    .register("(baz 1)", callback)
+    .build();
+
+IonReader ionReader = ion.newReader("{foo: 1}"


Please help me teach users to use an IonReaderBuilder for constructing IonReaders. I dream of deprecating all of the miscellaneous non-DOM-related utility methods from IonSystem one day.

Changed. Also added a test for the example to avoid breaking in the future, which already happened...

Also added a test for the example to make sure it works

tgregg

Before you merge, if you're going to make the first commit part of the history, don't forget to change it from "PathExtraction initinal implementaion" to "PathExtraction initial implementation".

tgregg · 2018-10-18T22:05:48Z

README.md

+```java 
+PathExtractorBuilder.standard()
+                    .withMatchCaseInsensitive(true) 
+                    .register("(foo)", (reader) -> { ... })


withSearchPath

tgregg · 2018-10-18T22:06:04Z

README.md

+
+### Notification  
+Each time the `PathExtractor` encounters a value that matches a registered search path it will invoke the respective 
+callback passing the reader positioned at the current value. See `PathExtractorBuilder#register` methods for more 


withSearchPath

PathExtraction initinal implementaion

33ad390

* Path Extraction implementation that exposes the IonReader to registered callbacks * Package setup, e.g. gradle, checkstyle, copyright

raganhan requested a review from tgregg October 17, 2018 02:41

tgregg reviewed Oct 17, 2018

View reviewed changes

raganhan added 10 commits October 17, 2018 16:54

Adding a user context example

304a356

Showing how you can use the lambda closure as ion-c user context

Wildcard escape should be first annotation

7a22315

making the wildcard escape annotation valid only as the first annotation. Added a test to show that

Make PathComponentParser static and adds a PathExtractor interface

aaaa169

PathExtractorImpl is now package private to make easier to add new implementations in the future if necessary

short circuiting zero search paths and test

7536374

Already worked, but short circuiting makes it clearer and avoids unecessary work. Also adds missing tests for this use case

Bugfix: fails when the callback return is higher than the reader

ec97718

relative depth

fixing initial Tracker size

90ab6dd

and some other spelling and missing comments

renaming builder's register methods to withSearchPath

9788b3a

using IonType.isContainer instead of hand coding container detection

0ef0b5d

adding support for specifying search paths as Ion lists

ae5525c

Addressing multiple PR comments

1fd9398

* Moving callback to SearchPath * Simplifying partial match detection logic * Fixing needToStepIn logic * Adding tests for nested search paths

tgregg reviewed Oct 18, 2018

View reviewed changes

Changing README example to not depend on IonSystem

9697962

Also added a test for the example to make sure it works

tgregg approved these changes Oct 18, 2018

View reviewed changes

README.md typos

f96a8f3

raganhan merged commit 864def4 into master Oct 18, 2018

raganhan deleted the initial-implementation branch October 18, 2018 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PathExtraction initinal implementaion #1

PathExtraction initinal implementaion #1

raganhan commented Oct 17, 2018

tgregg left a comment

tgregg Oct 17, 2018

raganhan Oct 17, 2018

tgregg Oct 17, 2018

raganhan Oct 17, 2018

tgregg Oct 17, 2018

raganhan Oct 17, 2018

tgregg Oct 17, 2018

raganhan Oct 17, 2018

tgregg Oct 17, 2018

raganhan Oct 17, 2018

tgregg Oct 17, 2018

raganhan Oct 18, 2018

tgregg Oct 17, 2018

tgregg Oct 17, 2018

raganhan Oct 18, 2018 •

edited

Loading

tgregg Oct 18, 2018

tgregg Oct 17, 2018

raganhan Oct 18, 2018

tgregg Oct 17, 2018

raganhan Oct 18, 2018

tgregg Oct 17, 2018

raganhan Oct 18, 2018 •

edited

Loading

raganhan commented Oct 18, 2018

tgregg Oct 18, 2018

raganhan Oct 18, 2018

tgregg left a comment

tgregg Oct 18, 2018

tgregg Oct 18, 2018


		private static final String WILDCARD_ESCAPE_ANNOTATION = "$ion_extractor_field";

		List<PathComponent> parse(final String ionPathExpression) {

PathExtraction initinal implementaion #1

PathExtraction initinal implementaion #1

Conversation

raganhan commented Oct 17, 2018

tgregg 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

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

Choose a reason for hiding this comment

raganhan Oct 18, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raganhan Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

raganhan commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgregg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raganhan Oct 18, 2018 •

edited

Loading

raganhan Oct 18, 2018 •

edited

Loading