Factor out ID + Module into a NodeId for different Node types

[fisherdarling]

Pull request

Description

This PR factors out many instances of id: String and module: Vec<String> into a new type, NodeId.

/// Identifies a node in the AST.
#[derive(Clone, Debug, PartialEq, Serialize)]
pub struct NodeId {
    /// The ID of the Node
    id: String,
    /// The module of the Node
    module: Vec<String>,
}

Thoughts / Main changes.

1. Factoring it out will break anything that relies on the serialization, #[serde(flatten)] will keep the same functionality. I did not add it in since I passed all of the tests.
2. I created a trait FullyQualifiedName with a fqn function that calculates the fully qualified name. I then replaced the fqXn methods with fqn. It feels cleaner.
3. NodeId feels like an ok name, could use suggestions.
4. Might get some performance boosts by switching to an inline string crate. Something like smartstring might help, I think it's worth making an issue for. You could also even make module an ArrayVec.

Related

• closes refactor out module + string for identifying enteties into a own struct #1211

Checklist

I'll update the changelog once we commit to the trait / naming.

• Update CHANGELOG.md appropriately, recording any changes, bug fixes, or other observable changes in behavior

Performance

[Licenser]

This looks great so far :D NodeId and fqn are good names, no objection there. I left a few comments in the code where I noticed things but there is really nothing major.

On smartstring and arrayvec worth a benchmark, but unless it's as measurable difference I think it'd be better to stick with build in types (less dependencies, less risks and less compile time 😂)

[coveralls]

Pull Request Test Coverage Report for Build 1440009724

• 61 of 61 (100.0%) changed or added relevant lines in 4 files are covered.
• 4 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.002%) to 87.276%

---

Files with Coverage Reduction	New Missed Lines	%
tremor-script/src/ast/eq.rs	1	67.99%
tremor-script/src/registry/custom_fn.rs	1	63.0%
tremor-value/src/value.rs	2	97.38%

Totals
Change from base Build 1433897190:	0.002%
Covered Lines:	18657
Relevant Lines:	21377

---

💛 - Coveralls

[fisherdarling]

This looks great so far :D NodeId and fqn are good names, no objection there. I left a few comments in the code where I noticed things but there is really nothing major.

On smartstring and arrayvec worth a benchmark, but unless it's as measurable difference I think it'd be better to stick with build in types (less dependencies, less risks and less compile time 😂)

I don't really know how "hot" tremor scripts get, so unless there's a lot of cloning / sharing in the interpreter then it's probably not worth it. Totally agree on the deps, risks, and compile time argument :P

[Licenser]

The script itself is barely ever cloned, and the NodeId is mostly for error handling needs (tell the user when something goes wrong where it gones wrong) during the execution phase it's never (at least I can't think of an instance) used.

[fisherdarling]

The script itself is barely ever cloned, and the NodeId is mostly for error handling needs (tell the user when something goes wrong where it gones wrong) during the execution phase it's never (at least I can't think of an instance) used.

Sounds good! Sorry It's taken me a bit to get back to this PR, moving to a new country :D

I'll be working on it today and hopefully finish it up.

[Licenser]

I love how this simplifies it down 👍 great work! It only needs a rebase to resolve the conflicts then it'll be good to go 🚀

[fisherdarling]

Alright, I think a rebased it properly, but there were some logic changes on main that may have been pulled over incorrectly. Mainly around the match on L173 of tremor-script/src/ast/query/raw.rs

[Licenser]

Sorry for the late reply on this, it seems like some of the test are failing at the current state :(

[fisherdarling]

Sorry for the late reply on this, it seems like some of the test are failing at the current state :(

No worries. I'll go through those tests and get them fixed this weekend.

[fisherdarling]

Alright, so I got clippy to run and cargo test --all --locked to pass. The only issue now, is that 3 cli integration tests do not pass.

I'm getting this error for test fg.12

After running: ./target/release/tremor test command tremor-cli/tests/cli --excludes postgres

Error: 
    1 | use std::string;
      |                 ^ Module `std/string` not found or not readable error in module path: 
    2 | 
    3 | let trimmed = string::trim(event.foo);

It seems to be an issue with path resolving. Thoughts on where the issue could lie? I think it has to do with pulling in upstream changes.

[mfelsche]

@fisherdarling you need to set the environmnt variable TREMOR_PATH to $PWD/tremor-script/lib (it needs to be an absolute path) so it points to the standard library location.

[fisherdarling]

It looks like it's just missing a couple of tests for node_id then?

[Licenser]

Ja given the number probably only a single one :D

[fisherdarling]

Alrighty! I added a test just pushed a squash commit. I think it's ready for a final CI run

[Licenser]

🚀 lovely improvement :D

[Licenser]

ah sod, didn't quite make it :(

[fisherdarling]

ah sod, didn't quite make it :(

That should do it!

[Licenser]

Done :D you're awesome @fisherdarling !

[fisherdarling]

Thank you @Licenser! Sorry that took a while!

[Licenser]

No reason to be sorry at all :) we appreciate the contribution a lot!