Skip to content
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

feat: implement full features as a derivative work of serde_json_path #20

Merged
merged 44 commits into from
Feb 13, 2025

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Feb 5, 2025

Modifications:

  • Drop serde related impls.
  • impl Ord for PathElement
  • Drop Integer wrapper (although it's a MUST in RFC 9535, I don't find the reason and highly suspect it's because JSON has only numbers (IEEE 754 float))
  • The function registry has been totally re-designed to generic over any VariantValue type.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
This reverts commit a0ac663.
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Comment on lines 39 to 46
pub fn parse_query_main(&self, mut input: TokenSlice) -> Result<Query, RefineError> {
(self.parse_root_query(), EOI)
.map(|(query, _)| query)
.parse_next(&mut input)
}

fn parse_root_query<'a>(&self) -> impl Parser<TokenSlice<'a>, Query, RefineError> {
move |input: &mut TokenSlice<'a>| text("$").map(|_| Query::default()).parse_next(input)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @epage!

Here when I use winnow, I need to parse a state FunctionRegistry top to down.

I wonder if we have better way to convey the state instead of impl Parser<TokenSlice<'a>, Query, Error> and move |input: &mut TokenSlice<'a>| every where.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you instead pass state through Stateful?

btw you can usually annotate your closures with just &mut _

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw you can usually annotate your closures with just &mut _

Sounds good.

Can you instead pass state through Stateful?

I try this in the latest commit but it fails to compile:

error[E0599]: the method `map` exists for tuple `(fn(&mut Stateful<..., ...>) -> ... {parse_root_query::<...>}, ...)`, but its trait bounds were not satisfied
  --> spath/src/parser/parse.rs:43:10
   |
42 | /     (parse_root_query, EOI)
43 | |         .map(|(query, _)| query)
   | |         -^^^ method cannot be called due to unsatisfied trait bounds
   | |_________|
   |
   |

error[E0596]: cannot borrow data in dereference of `Stateful<winnow::stream::TokenSlice<'_, Token<'_>>, ParseContext<R>>` as mutable
  --> spath/src/parser/parse.rs:48:52
   |
48 |     text("$").map(|_| Query::default()).parse_next(input)
   |                                                    ^^^^^ cannot borrow as mutable
   |
   = help: trait `DerefMut` is required to modify through a dereference, but it is not implemented for `Stateful<winnow::stream::TokenSlice<'_, Token<'_>>, ParseContext<R>>`

Not quite sure how to properly type it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second error is a type mismatch between text (TokenStream) and parse_root_query (Stateful<_, _>)

For the first, did you implement Parser for `TokenKind? Not too easy to search for that in the browser. If so, you likely also need to make the Input type generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage Thanks for your information! All works well now.

I'd finish the filter expr parsing part and then dig into how to properly report the parse error.

I've made a span-based error reporting strategy elsewhere with nom, and would like to see how to reimplement it with winnow.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Comment on lines 489 to 514
let registry = input.state.registry();

(
Identifier,
delimited(
text("("),
separated(0.., parse_function_argument, text(",")),
text(")"),
),
)
.try_map(|(name, args)| {
let name = name.text().to_string();
let args: Vec<FunctionExprArg> = args;
let function = match registry.get(name.as_str()) {
Some(function) => function,
None => return Err(FunctionValidationError::Undefined { name }),
};
function.validate(args.as_slice(), registry.as_ref())?;
Ok(FunctionExpr {
name,
args,
return_type: function.result_type(),
})
})
.parse_next(input)
}
Copy link
Contributor Author

@tisonkun tisonkun Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage I notice a wart of Stateful that I cannot get a ref in mapper because parse_next needs a mutable ref.

I must wrap the registry in Arc as:

pub type TokenSlice<'a> = winnow::stream::TokenSlice<'a, Token<'a>>;

pub type Input<'a, Registry> = Stateful<TokenSlice<'a>, InputState<Registry>>;

pub struct InputState<Registry> {
    registry: Arc<Registry>,
}

impl<Registry> InputState<Registry> {
    pub fn new(registry: Registry) -> Self {
        let registry = Arc::new(registry);
        Self { registry }
    }

    pub fn registry(&self) -> Arc<Registry> {
        self.registry.clone()
    }

    pub fn into_registry(self) -> Registry {
        // SAFETY: The registry is only owned by this instance.
        Arc::into_inner(self.registry).unwrap()
    }
}

If I use barely Registry and return &self.registry in input.state.registry(), a compile error would occur:

error[E0502]: cannot borrow `*input` as mutable because it is also borrowed as immutable
   --> spath/src/parser/parse.rs:491:5
    |
489 |       let registry = input.state.registry();
    |                      ----------- immutable borrow occurs here
490 |
491 | /     (
492 | |         Identifier,
493 | |         delimited(
494 | |             text("("),
...   |
512 | |         })
513 | |         .parse_next(input)
    | |__________----------______^ mutable borrow occurs here
    |            |
    |            immutable borrow later used by call

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like the core of the issue is with accessing InputState inside of try_map. Our issue for this is winnow-rs/winnow#231.

A workaround is to not use try_map

    let (name, args) = (
        Identifier,
        delimited(
            text("("),
            separated(0.., parse_function_argument, text(",")),
            text(")"),
        ),
    ).parse_next(input)?;

            let name = name.text().to_string();
            let args: Vec<FunctionExprArg> = args;
            let function = match registry.get(name.as_str()) {
                Some(function) => function,
                None => return Err(FunctionValidationError::Undefined { name }),
            };
            function.validate(args.as_slice(), registry.as_ref())?;
            Ok(FunctionExpr {
                name,
                args,
                return_type: function.result_type(),
            })

The only limitation being that you will need to manually convert the error type

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

Thanks a lot for your help @epage!

I use the workaround and all the happy path looks good now.

I'd merge this one and work on error reporting part. Basically, I'd track all the span and report error with context error, like:

error: failed to execute statement
  --> ScopeQL:1:8
  |
1 | $0[42::]
  | -  --  ^ unexpected `]`, expecting `INT`, `UINT`, `FLOAT`, `STRING`, `BOOLEAN`, `TIMESTAMP`, `INTERVAL`, or `VARIANT`
  | |  |    
  | |  while parsing expression
  | while parsing expression

But perhaps a more simple version.

@tisonkun tisonkun merged commit ba41f50 into main Feb 13, 2025
9 checks passed
@tisonkun tisonkun deleted the full-features branch February 13, 2025 03:21
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.

2 participants