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

chore: multiple improvements #935

Closed
wants to merge 2 commits into from
Closed

Conversation

ByteBaker
Copy link

It's an overall negative PR, meaning there are more lines deleted than added. Following changes have been made, some of which should actually improve the performance all so slightly.

  • remove a few unwrap(), partly covers Replace unwrap() #933
  • replace code with idiomatic version of it
  • rewrite time_from_path to make it simpler + robust
  • rewrite flatten_objects_for_count reducing loops and simplify the accumulator inside it
  • make the Regex in Message::extract_column_names static
  • correct a few panic messages

Description


This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Copy link
Contributor

github-actions bot commented Sep 22, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@ByteBaker
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@ByteBaker
Copy link
Author

recheck

nitisht added a commit to parseablehq/.github that referenced this pull request Sep 22, 2024
It's an overall negative PR, meaning there are more lines deleted than
added. Following changes have been made:

- remove a few `unwrap()`, partly covers parseablehq#933
- replace code with idiomatic version of it
- rewrite `time_from_path` to make it simpler + robust
- rewrite `flatten_objects_for_count` reducing loops and simplify the
  accumulator inside it
- make the Regex in `Message::extract_column_names` static
- correct a few panic messages
@nikhilsinhaparseable
Copy link
Contributor

@ByteBaker Integration tests fail in the PR, and if you look in the details, Quest Integrity test fails as the ingestion fails with the error -
Error in response: Invalid(Schema mismatch)
Clone the quest repo from https://github.com/parseablehq/quest
and run the Parseable server locally.
Execute the TestIntegrity function under integrity_test.go to reproduce the error.

Looks like there is some issue with the change you did in one of the two methods -
fields_mismatch and valid_type

The same test runs successfully in the main branch, you can verify the same.

Thanks!

@nitisht
Copy link
Member

nitisht commented Oct 12, 2024

Closing because of no response. @ByteBaker feel free to reopen if you plan to work on this.

@nitisht nitisht closed this Oct 12, 2024
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.

3 participants