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

Fixes parameter after fetch #4086

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Vignesh1-art
Copy link

Thank you for submitting this pull request! We really appreciate you spending the time to work on these changes.

What is the motivation?

Parsing fails if variables are encountered after FETCH statement

What does this change do?

Fixes this problem. I have added new parsing function to parse list of variables and Parses it after FETCH statement and implemented to compute variable

What is your testing strategy?

Added unit tests

Is this related to any issues?

#3028

Does this change need documentation?

If this pull request requires changes, updates, or improvements to the documentation, then add a corresponding issue on the docs.surrealdb.com repository, and link to it here.

  • No documentation needed

Have you read the Contributing Guidelines?

@Vignesh1-art
Copy link
Author

Vignesh1-art commented May 23, 2024

@tobiemh This PR has required test case. Can you please review it.

@Vignesh1-art
Copy link
Author

@tobiemh or @DelSkayn someone Please review and approve the workflow

@Vignesh1-art
Copy link
Author

I have fixed formatting issues, @tobiemh or @DelSkayn can you please approve the workflow

@Vignesh1-art
Copy link
Author

Also @tobiemh or @DelSkayn can you please tell me how to check for formatting issues in local?

@gguillemas
Copy link
Contributor

Hi @Vignesh1-art. We really appreciate your contribution. Please, be patient and do not repeatedly ping people, they have personal lives and can be sick, on holidays or just busy with something else. You can check format locally using cargo fmt --all --check, which will also automatically fix the format if you remove --check.

@Vignesh1-art
Copy link
Author

Vignesh1-art commented May 28, 2024

Hi @Vignesh1-art. We really appreciate your contribution. Please, be patient and do not repeatedly ping people, they have personal lives and can be sick, on holidays or just busy with something else. You can check format locally using cargo fmt --all --check, which will also automatically fix the format if you remove --check.

I will take care of it from next time! Thanks @gguillemas, I ran formatting checks in local and now no issues found. If you could approve the workflow please do it.

@Vignesh1-art
Copy link
Author

Vignesh1-art commented May 29, 2024

I rebased the branch and did not try to compile it, MIne bad sorry! Now It is working

Comment on lines +38 to +47
Part::Value(f) => {
let x = stk.run(|stk| f.compute(stk, ctx, opt, None)).await?;
match x {
Value::Strand(s) => match v.get_mut(s.as_str()) {
Some(v) => stk.run(|stk| v.fetch(stk, ctx, opt, path)).await,
_ => Ok(()),
},
_ => Ok(()),
}
}
Copy link
Member

@tobiemh tobiemh May 29, 2024

Choose a reason for hiding this comment

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

Hi @Vignesh1-art, although this PR works, the way it is implemented probably isn't correct. With this PR, an idiom part can now be a param or a value, meaning that we have to compute a path part, check if it is a string, and process a string as if it is an idiom part. This isn't an ideal way of doing things (although it does work)!

Ideally, we would actually need to accept a Value using parse_value within the try_parse_fetch function in the core/src/syn/parser/stmt/part.rs file:

pub async fn try_parse_fetch(&mut self, ctx: &mut Stk) -> ParseResult<Option<Fetchs>> {
    if !self.eat(t!("FETCH")) {
        return Ok(None);
    }
    let v = self.parse_idiom_list(ctx).await?.into_iter().map(Fetch).collect();
    Ok(Some(Fetchs(v)))
}

And then we would have to change the Fetch type in the core/src/sql/fetch.rs file so that a Fetch is a Value and not an Idiom.

pub struct Fetch(pub Idiom);

Then finally, we would have to ensure that the FETCH clause is computed before being processed.

@Vignesh1-art Vignesh1-art requested a review from tobiemh May 31, 2024 12:18
@Vignesh1-art
Copy link
Author

@tobiemh Can you suggest please?

@tobiemh
Copy link
Member

tobiemh commented Jun 5, 2024

Hi @Vignesh1-art changes would need to be made according to this review here: #4086 (comment)

@Vignesh1-art
Copy link
Author

@tobiemh Yes i have made changes according that comment, but since I have changed Fetch(Idiom) to Fetch(Value) I can't pass it to compute of fetch statement, because it is of not type Part, mine question is should write logic to convert Value to Part? So logic is

if Value is idiom then
converting to part is straight forward
else
compute Value this will return Strand, Number or Something
Convert that to Part.

Is that correct @tobiemh ?

@@ -239,6 +239,7 @@ impl Parser<'_> {
let graph = ctx.run(|ctx| self.parse_graph(ctx, Dir::In)).await?;
Part::Graph(graph)
}
t!("$param") => Part::Value(Value::Param(self.next_token_value()?)),
Copy link
Member

Choose a reason for hiding this comment

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

Adding support for a parameter here doesn't just change the FETCH clause to support params but changes every place where a idiom is allowed which is a lot of places, most of which I am not sure we want to support having parameters in those positions.

Instead of adding support here for parameters I think a better approach would be to implement it in core::syn::parser::stmt::parts::try_parse_fetch. That way the change is limited to just the FETCH clause.

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.

None yet

4 participants