Skip to content

Simpler Catalog implementation#11

Merged
KKould merged 4 commits intoKipData:mainfrom
KKould:refactor/catalog_simplification
Jun 26, 2023
Merged

Simpler Catalog implementation#11
KKould merged 4 commits intoKipData:mainfrom
KKould:refactor/catalog_simplification

Conversation

@KKould
Copy link
Copy Markdown
Member

@KKould KKould commented Jun 22, 2023

What problem does this PR solve?

re-implement the Catalog and simplify it by removing the Catalog of DataBase and Schema

What is changed and how it works?

  • removed catalog/databse.rs and catalog/schema.rs
  • simplify the implementation of Catalog
  • Use third-party crates to implement id generation (Tips: If you consider that the ids of restarts are still ordered and non-repeating, here is Fixme)
  • remove the original redundant thread safety

Code changes

  • Has Rust code change

@KKould KKould added the invalid This doesn't seem right label Jun 22, 2023
@KKould KKould requested review from eliasyaoyc and loloxwg June 22, 2023 16:03
@KKould KKould self-assigned this Jun 22, 2023
@cr-gpt
Copy link
Copy Markdown

cr-gpt Bot commented Jun 22, 2023

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

@KKould KKould linked an issue Jun 26, 2023 that may be closed by this pull request
13 tasks
Copy link
Copy Markdown
Member

@loloxwg loloxwg left a comment

Choose a reason for hiding this comment

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

  1. Delete failed tests
  2. Cargo fmt

…d generation to avoid third-party dependencies
Comment thread Cargo.toml
integer-encoding = "3.0.4"

[dev-dependencies]
ctor = "0.2.0"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Based on the provided code patch, here are some observations and suggestions for improvement:

  • There is no actual code in the patch, it only shows changes made to the project's Cargo.toml file.
  • The removal of two blank lines at the beginning of the file does not affect functionality but can improve readability.
  • The removal of the doctest option from the [lib] section does not have any impact if there were no doctests in the library. Otherwise, it should be investigated why this change was made.
  • It is recommended to add version numbers for all dependencies to ensure consistent builds over time. It's good to see that most dependencies already have a version specified.
  • The addition of the integer-encoding dependency looks valid based on its version and purpose, assuming it is needed in the codebase.
  • The [dev-dependencies] section contains only one package, which is used during development, so it does not affect production builds. However, it's important to make sure that all development dependencies are not causing security risks or any performance issues.

Overall, the changes in the code patch seem reasonable and appropriate, but without more context, it's hard to provide a more precise review.

@eliasyaoyc
Copy link
Copy Markdown
Contributor

lgtm

Comment thread src/types/mod.rs Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The code appears to be using atomic operations for generating and encoding IDs. This can help ensure that the IDs are unique and consistent under concurrent access. However, there are a few points for improvement:

  1. The code lacks comments, which can make it hard for others to understand its purpose and how it works. Adding some comments would improve the readability.

  2. There is no error handling mechanism in IdGenerator. If fetching the next ID fails for any reason, such as running out of IDs or a system failure, the application will panic. It's recommended to handle possible errors gracefully, return result types from functions, and/or log possible failures.

  3. The Ignore attribute for the test case is worrisome -- if a test doesn't need to run, it should be removed instead of ignored. Also, it might be more appropriate to use a separate database for unit testing than modifying global static values.

  4. The #[cfg(test)] attribute on the module implies that some names are accessible only within tests. While this semantics are required, it's good practice to have clear separation, documentation, and don't expose testing-only dependencies publicly.

  5. The naming convention could be improved. For example, build method in the IdGenerator might not immediately suggest what it does. Also, the DataTypeKind could be called something more descriptive, like SqlDataType.

  6. The encoding and decoding methods should probably have comments describing which format is being used and why.

@KKould KKould merged commit 1a36c93 into KipData:main Jun 26, 2023
@loloxwg loloxwg linked an issue Jun 26, 2023 that may be closed by this pull request
3 tasks
@loloxwg loloxwg removed a link to an issue Jun 26, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

catalog

3 participants