Skip to content

Conversation

@max-sixty
Copy link
Member

@max-sixty max-sixty commented May 3, 2023

As I mentioned on Discord. Inspired by this discussion

  • This is me trying to become more acquainted with the code
  • We don't have to merge it (genuinely)
  • Though I do find this code simpler to understand
  • It's less impressive rust, maybe less abstract, less use of the type system
  • It's the same verbosity, as measured by lines of code
  • It does more allocating, more .clone()
  • But it's actually slightly faster on cargo bench, about 3% on my machine
    • It doesn't run each try_into_* before doing the next one if that fails
    • Maybe it allows for fewer eventual optimizations in the future though?
  • It would need a quick pass to add the regex from today

GPT-4 helped me a lot!


  • I added a couple of notes on further changes. We're probably doing too much if args.len() == X in lots of places, and could have these as structs & enums in rust, though I'm not sure how deep in the compiler we want them (maybe there's a difference between functions & operators?)

@max-sixty max-sixty requested a review from aljazerzen May 3, 2023 05:46
Comment on lines +287 to +300
"std.mul" => Some(Multiply),
"std.div" => Some(Divide),
"std.mod" => Some(Modulo),
"std.add" => Some(Plus),
"std.sub" => Some(Minus),
"std.eq" => Some(Eq),
"std.ne" => Some(NotEq),
"std.gt" => Some(Gt),
"std.lt" => Some(Lt),
"std.gte" => Some(GtEq),
"std.lte" => Some(LtEq),
"std.and" => Some(And),
"std.or" => Some(Or),
"std.concat" => Some(StringConcat),
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, this would use std::STD_X so we have it "linked" with RQ.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I left some notes here, I agree the current state isn't ideal. I'm not sure how much we want to have these deep in the compiler with something like an Enum...

Copy link
Member

@aljazerzen aljazerzen left a comment

Choose a reason for hiding this comment

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

This is way better, as in way more readable.

I went too deep with the no-clone frenzy :D It's actually slower probably because it was searching for impl of functions in linear search. This PR is now using sub-logarithmic lookups (because it's using match).

@max-sixty
Copy link
Member Author

This is way better, as in way more readable.

I went too deep with the no-clone frenzy :D It's actually slower probably because it was searching for impl of functions in linear search. This PR is now using sub-logarithmic lookups (because it's using match).

OK great! Thanks for being open. I was holding that it was "worse but easier rust" — maybe it's actually / actually much worse. I do think this sort of change can make it easier for others to contribute...

I'll update to yesterday's regex addition, resolve hte comments, and leave a couple of notes re strings vs FunctionDecls (i.e. #2532 (comment))

@max-sixty
Copy link
Member Author

...and using slice patterns reduces some of the if args.len... verbosity: e6fc1f2

@max-sixty
Copy link
Member Author

I'll merge given your ✅ , but happy to do follow-up with any other changes

@max-sixty max-sixty merged commit 19426e8 into PRQL:main May 4, 2023
@max-sixty max-sixty deleted the refactor branch May 4, 2023 02:02
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