mattbishop / sql-jsonpath-js

JS implementation of the SQL/JSONPath dialect, from SQL2016.
MIT License
0 stars 0 forks source link

Parse 'strict' and 'lax' keywords at the start of a statement #14

Closed mattbishop closed 2 years ago

mattbishop commented 2 years ago

Resolves #7

mattbishop commented 2 years ago

@JeffML want to review?

JeffML commented 2 years ago

Sure, I'll take a look soonish.

On Tue, Feb 8, 2022 at 10:28 AM Matt Bishop @.***> wrote:

@JeffML https://github.com/JeffML want to review?

— Reply to this email directly, view it on GitHub https://github.com/mattbishop/sql-jsonpath-js/pull/14#issuecomment-1032930046, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2VY3I3HUPL3JFGCUGSQH3U2FOGXANCNFSM5N3IHPHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

JeffML commented 2 years ago

Looks like the mode is captured ok. Should lax be the default? I see that if mode is unspecified it is "undefined", but I think that implies lax.

There's the issue of nesting/unnesting arrays in lax mode (6.3, table 28). I don't know what operator subrules (if any) require conditionals to handle those cases.

Error handling for non-null data in strict mode is something you might want to write an issue about. I haven't given it any thought.

mattbishop commented 2 years ago

lax is the default, according to the spec. It makes sense to set mode to always be lax if not set, I will add that in.

As for interpreting the meaning of strict, that is a runtime concern, not a parser concern. When one applies a parsed JSONPath statement to a JSON object, these modes kick in.

JeffML commented 2 years ago

Hmmm...I still think the wrapping/unwrapping of arrays might be handled at the parser level in lax mode, but it does make for a more complicated parser.

On Wed, Feb 9, 2022, 11:46 AM Matt Bishop @.***> wrote:

lax is the default, according to the spec. It makes sense to set mode to always be lax if not set, I will add that in.

As for interpreting the meaning of strict, that is a runtime concern, not a parser concern. When one applies a parsed JSONPath statement to a JSON object, these modes kick in.

— Reply to this email directly, view it on GitHub https://github.com/mattbishop/sql-jsonpath-js/pull/14#issuecomment-1034131193, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2VY3L4Q5C74ENOT6NJHC3U2LACNANCNFSM5N3IHPHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.***>

mattbishop commented 2 years ago

I added 'lax' as the default mode if none is specified.

mattbishop commented 2 years ago

About the lax array wrapping/unwrapping, how would that affect the statement parser's CST output? For instance:

lax $.track.segments.location

Which of those parts is an array? We don't know, so the CST would have to have some kind of OR declaration to indicate either property accessor or array[*] accessor for every path part. That might be a bit heavy?

Perhaps the search function could pick up the load and convert the property to an array handler if it discovers the property type is actually an array instead of object. Something like:

if (lhsElement.property) {
  const property = obj[lhsElement.property]
  if (Array.isArray(property)) {
    if (ctx.mode === "strict") {
      throw new Error("strict mode won't unwrap arrays for you")
    }
    // switch to property[*] handling
  } else {
    // handle as a single property 
  }
}
JeffML commented 2 years ago

Sorry, didn't see your previous comments until just now.

On Mon, Feb 14, 2022 at 10:36 AM Matt Bishop @.***> wrote:

Merged #14 https://github.com/mattbishop/sql-jsonpath-js/pull/14 into main.

— Reply to this email directly, view it on GitHub https://github.com/mattbishop/sql-jsonpath-js/pull/14#event-6069342241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2VY3KEJMD4ON7ZTUWHYO3U3FDSTANCNFSM5N3IHPHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.***>