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

feat(file_index): add file index other interfaces #68

Conversation

devillove084
Copy link
Contributor

part #34

@Xuanwo
Copy link
Member

Xuanwo commented Sep 11, 2024

Hi, @devillove084, thanks a lot for your effort. However, I have some concerns that we are not heading in the right direction. Translating every Java class into a trait and using Box<dyn Trait> doesn't seem ideal to me now. I'm wondering if it's possible to implement a real file index struct first and then consider how to expose them.

@devillove084
Copy link
Contributor Author

Hi, @devillove084, thanks a lot for your effort. However, I have some concerns that we are not heading in the right direction. Translating every Java class into a trait and using Box<dyn Trait> doesn't seem ideal to me now. I'm wondering if it's possible to implement a real file index struct first and then consider how to expose them.

I must admit that this additional work stems from my own lack of engineering rigor. Given the current situation, the entire system design needs to be approached from the ground up. I agree with your perspective, and I think it’s best to start by focusing on the specific details. Therefore, I’ll close this PR and redesign it with a dedicated BitMap index and BloomFilter index in mind.

Thank you for taking the time to review it—I’ve learned a great deal from your feedback.

@devillove084
Copy link
Contributor Author

Close it and move on!

@devillove084 devillove084 deleted the feat/impl_file_index_other_interfaces branch September 11, 2024 14:11
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