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.

Sign up for GitHub

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

Jump to bottom

Modernize LineFileDocs. #12929

Merged
merged 1 commit into from Dec 19, 2023
Merged

Modernize LineFileDocs. #12929

merged 1 commit into from Dec 19, 2023

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Dec 12, 2023

This replaces StringField/SortedDocValuesField with KeywordField and IntPoint/NumericDocValuesField with IntField.

@jpountz
Modernize LineFileDocs.
a82aa9b 
This replaces `StringField`/`SortedDocValuesField` with `KeywordField` and
`IntPoint`/`NumericDocValuesField` with `IntField`.
mikemccand
mikemccand approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@mikemccand mikemccand left a comment

Choose a reason for hiding this comment

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

Thanks @jpountz. I wonder if luceneutil's hard fork of this source file is also rusty in the same way?

@jpountz
Copy link
Contributor Author

jpountz commented Dec 13, 2023

@mikemccand luceneutil is better at remaining up-to-date with Lucene than Lucene itself :) mikemccand/luceneutil@76ff349 mikemccand/luceneutil@bb071f7

@mikemccand
Copy link
Member

@mikemccand luceneutil is better at remaining up-to-date with Lucene than Lucene itself :) mikemccand/luceneutil@76ff349 mikemccand/luceneutil@bb071f7

Ha! Wild :)

Maybe we should unfork? Why does luceneutil need a hard fork here ... couldn't it just depend on Lucene's test-framework to pull this dep in?

@jpountz
Copy link
Contributor Author

jpountz commented Dec 19, 2023

From a quick check, the luceneutil version has much more complexity around indexing facets, doc values, etc. It's not entirely obvious to me if sharing the code would help or hurt.

@jpountz jpountz merged commit bcc7e12 into apache:main Dec 19, 2023
4 checks passed
@jpountz jpountz deleted the modernize_line_file_docs branch December 19, 2023 10:25
jpountz added a commit that referenced this pull request Dec 19, 2023
@jpountz
Modernize LineFileDocs. (#12929)
a63d459 
This replaces `StringField`/`SortedDocValuesField` with `KeywordField` and
`IntPoint`/`NumericDocValuesField` with `IntField`.
@s1monw
Copy link
Member

s1monw commented Jan 16, 2024

I was working on adding some new BWC tests for the parent field and it seems BWC index creation and testing is relying on some things that changed here. Just flagging this for now while I work on a fix.

s1monw added a commit to s1monw/lucene that referenced this pull request Jan 17, 2024
@s1monw
Fix BWC test generation after mondernizing LineDocFile
f946339 
The changes on apache#12929 broke the generation code for BWC indices
since they are expecting vertain fields created by LineDocFile.
Yet, this change adds some sanity checks that run with unittest to ensure
the bwc generatin is at least readable with the current version.

Relates to apache#12929
@s1monw s1monw mentioned this pull request Jan 17, 2024
Merged
s1monw added a commit that referenced this pull request Jan 17, 2024
@s1monw
Fix BWC test generation after mondernizing LineDocFile (#13021)
c661c88 
The changes on #12929 broke the generation code for BWC indices
since they are expecting vertain fields created by LineDocFile.
Yet, this change adds some sanity checks that run with unittest to ensure
the BWC generation is at least readable with the current version.

Relates to #12929
s1monw added a commit that referenced this pull request Jan 17, 2024
@s1monw
Fix BWC test generation after mondernizing LineDocFile (#13021)
564219a 
The changes on #12929 broke the generation code for BWC indices
since they are expecting vertain fields created by LineDocFile.
Yet, this change adds some sanity checks that run with unittest to ensure
the BWC generation is at least readable with the current version.

Relates to #12929
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikemccand mikemccand mikemccand approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jpountz @mikemccand @s1monw