You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "JarvisCraft (via GitHub)" <gi...@apache.org> on 2023/05/15 22:49:28 UTC
[GitHub] [lucene] JarvisCraft opened a new pull request, #12296: Seal `IndexReaderContext`
JarvisCraft opened a new pull request, #12296:
URL: https://github.com/apache/lucene/pull/12296
### Description
`IndexReaderContext` is already effectively sealed since it's constructor does type check throwing `Error` if `this` is neither instance of `CompositeReaderContext` nor `LeafReaderContext`.
This PR simply makes this restriction explicit (codewise) turning it from a runtime error into a compile time one to extend this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #12296: Seal `IndexReader` and `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler merged PR #12296:
URL: https://github.com/apache/lucene/pull/12296
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549878949
Please remove the protected from consructor. We don't want to show it in javadocs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12296:
URL: https://github.com/apache/lucene/pull/12296#discussion_r1195218209
##########
lucene/core/src/java/org/apache/lucene/index/IndexReaderContext.java:
##########
@@ -22,27 +22,25 @@
* A struct like class that represents a hierarchical relationship between {@link IndexReader}
* instances.
*/
-public abstract class IndexReaderContext {
+public abstract sealed class IndexReaderContext permits CompositeReaderContext, LeafReaderContext {
/** The reader context for this reader's immediate parent, or null if none */
public final CompositeReaderContext parent;
/**
- * <code>true</code> if this context struct represents the top level reader within the
+ * {@code true} if this context struct represents the top level reader within the
* hierarchical context
*/
public final boolean isTopLevel;
- /** the doc base for this reader in the parent, <code>0</code> if parent is null */
+ /** the doc base for this reader in the parent, {@code 0} if parent is null */
public final int docBaseInParent;
- /** the ord for this reader in the parent, <code>0</code> if parent is null */
+ /** the ord for this reader in the parent, {@code 0} if parent is null */
public final int ordInParent;
// An object that uniquely identifies this context without referencing
// segments. The goal is to make it fine to have references to this
// identity object, even after the index reader has been closed
- final Object identity = new Object();
+ protected final Object identity = new Object();
Review Comment:
I think this is an unrelated change. Do we need to make it protected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549737196
Hi this is a good idea for Lucene 10.0 (minimum is Java 17).
This can't be backported to Java 11, so won't be on Lucene 9.x.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on pull request #12296: Seal `IndexReaderContext`
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549897649
Ready:
* brought back package-private visibility;
* applied the same change to `IndexReader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549871892
Could you apply the same change to `IndexReader`; it should be sealed, too? https://github.com/apache/lucene/blob/f53eb28af053d7612f7e4d1b2de05d33dc410645/lucene/core/src/java/org/apache/lucene/index/IndexReader.java#L72-L76
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12296: Seal `IndexReader` and `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1550842694
Thanks @JarvisCraft, I merged it to main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549747674
Please run `gradlew tidy`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on pull request #12296: Seal `IndexReaderContext`
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549819407
> Please run gradlew tidy.
All done :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12296:
URL: https://github.com/apache/lucene/pull/12296#discussion_r1195220727
##########
lucene/core/src/java/org/apache/lucene/index/IndexReaderContext.java:
##########
@@ -22,27 +22,25 @@
* A struct like class that represents a hierarchical relationship between {@link IndexReader}
* instances.
*/
-public abstract class IndexReaderContext {
+public abstract sealed class IndexReaderContext permits CompositeReaderContext, LeafReaderContext {
/** The reader context for this reader's immediate parent, or null if none */
public final CompositeReaderContext parent;
/**
- * <code>true</code> if this context struct represents the top level reader within the
+ * {@code true} if this context struct represents the top level reader within the
* hierarchical context
*/
public final boolean isTopLevel;
- /** the doc base for this reader in the parent, <code>0</code> if parent is null */
+ /** the doc base for this reader in the parent, {@code 0} if parent is null */
public final int docBaseInParent;
- /** the ord for this reader in the parent, <code>0</code> if parent is null */
+ /** the ord for this reader in the parent, {@code 0} if parent is null */
public final int ordInParent;
// An object that uniquely identifies this context without referencing
// segments. The goal is to make it fine to have references to this
// identity object, even after the index reader has been closed
- final Object identity = new Object();
+ protected final Object identity = new Object();
Review Comment:
Ah you mentioned it. I don't remember why this `identity` is there. Is it really needed to be public. I'd keep this as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on a diff in pull request #12296: Seal `IndexReaderContext`
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on code in PR #12296:
URL: https://github.com/apache/lucene/pull/12296#discussion_r1195281538
##########
lucene/core/src/java/org/apache/lucene/index/IndexReaderContext.java:
##########
@@ -22,27 +22,25 @@
* A struct like class that represents a hierarchical relationship between {@link IndexReader}
* instances.
*/
-public abstract class IndexReaderContext {
+public abstract sealed class IndexReaderContext permits CompositeReaderContext, LeafReaderContext {
/** The reader context for this reader's immediate parent, or null if none */
public final CompositeReaderContext parent;
/**
- * <code>true</code> if this context struct represents the top level reader within the
+ * {@code true} if this context struct represents the top level reader within the
* hierarchical context
*/
public final boolean isTopLevel;
- /** the doc base for this reader in the parent, <code>0</code> if parent is null */
+ /** the doc base for this reader in the parent, {@code 0} if parent is null */
public final int docBaseInParent;
- /** the ord for this reader in the parent, <code>0</code> if parent is null */
+ /** the ord for this reader in the parent, {@code 0} if parent is null */
public final int ordInParent;
// An object that uniquely identifies this context without referencing
// segments. The goal is to make it fine to have references to this
// identity object, even after the index reader has been closed
- final Object identity = new Object();
+ protected final Object identity = new Object();
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549883736
Please don't change public javadocs, so just make the classes sealed and keep documentation the same. Only remove runtime check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12296: Seal `IndexReader` and `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1551296276
Javadocs look fine: https://ci-builds.apache.org/job/Lucene/job/Lucene-Artifacts-main/javadoc/core/org/apache/lucene/index/IndexReader.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12296: Seal `IndexReaderContext`
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12296:
URL: https://github.com/apache/lucene/pull/12296#discussion_r1195330910
##########
lucene/core/src/java/org/apache/lucene/index/IndexReaderContext.java:
##########
@@ -22,27 +22,26 @@
* A struct like class that represents a hierarchical relationship between {@link IndexReader}
* instances.
*/
-public abstract class IndexReaderContext {
+public abstract sealed class IndexReaderContext permits CompositeReaderContext, LeafReaderContext {
/** The reader context for this reader's immediate parent, or null if none */
public final CompositeReaderContext parent;
/**
- * <code>true</code> if this context struct represents the top level reader within the
- * hierarchical context
+ * {@code true} if this context struct represents the top level reader within the hierarchical
+ * context
*/
public final boolean isTopLevel;
- /** the doc base for this reader in the parent, <code>0</code> if parent is null */
+ /** the doc base for this reader in the parent, {@code 0} if parent is null */
public final int docBaseInParent;
- /** the ord for this reader in the parent, <code>0</code> if parent is null */
+ /** the ord for this reader in the parent, {@code 0} if parent is null */
public final int ordInParent;
// An object that uniquely identifies this context without referencing
// segments. The goal is to make it fine to have references to this
// identity object, even after the index reader has been closed
final Object identity = new Object();
- IndexReaderContext(CompositeReaderContext parent, int ordInParent, int docBaseInParent) {
- if (!(this instanceof CompositeReaderContext || this instanceof LeafReaderContext))
- throw new Error("This class should never be extended by custom code!");
+ protected IndexReaderContext(
Review Comment:
Please make it package protected again, as we don't want it to appear in Javadocs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on pull request #12296: Seal `IndexReader` and `IndexReaderContext`
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on PR #12296:
URL: https://github.com/apache/lucene/pull/12296#issuecomment-1549969315
> Do you want to add a CHANGES.txt entry
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org