[jira] [Commented] (LUCENE-8805) Parameter changes for binaryField() and stringField() in StoredFieldVisitor
[
https://issues.apache.org/jira/browse/LUCENE-8805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16844085#comment-16844085
]
Namgyu Kim commented on LUCENE-8805:
Sure. I will just work on stringField and update the patch.
I'll keep the TODO comment on binaryField.
Do we need additional comments on binaryField?
I will reflect if you tell me.
> Parameter changes for binaryField() and stringField() in StoredFieldVisitor
> ---
>
> Key: LUCENE-8805
> URL: https://issues.apache.org/jira/browse/LUCENE-8805
> Project: Lucene - Core
> Issue Type: Improvement
>Reporter: Namgyu Kim
>Priority: Major
> Attachments: LUCENE-8805.patch
>
>
> I wrote this patch after seeing the comments left by [~mikemccand] when
> SortingStoredFieldsConsumer class was first created.
> {code:java}
> @Override
> public void binaryField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new BR here?
> ...
> }
> @Override
> public void stringField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new String here?
> ...
> }
> {code}
> I changed two things.
> 1) change binaryField() parameters from byte[] to BytesRef.
> 2) change stringField() parameters from byte[] to String.
> I also changed the related contents while doing the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (LUCENE-8805) Parameter changes for binaryField() and stringField() in StoredFieldVisitor
[
https://issues.apache.org/jira/browse/LUCENE-8805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16844072#comment-16844072
]
Robert Muir commented on LUCENE-8805:
-
We could do the String case here and leave the BytesRef case as a TODO for
another issue?
> Parameter changes for binaryField() and stringField() in StoredFieldVisitor
> ---
>
> Key: LUCENE-8805
> URL: https://issues.apache.org/jira/browse/LUCENE-8805
> Project: Lucene - Core
> Issue Type: Improvement
>Reporter: Namgyu Kim
>Priority: Major
> Attachments: LUCENE-8805.patch
>
>
> I wrote this patch after seeing the comments left by [~mikemccand] when
> SortingStoredFieldsConsumer class was first created.
> {code:java}
> @Override
> public void binaryField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new BR here?
> ...
> }
> @Override
> public void stringField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new String here?
> ...
> }
> {code}
> I changed two things.
> 1) change binaryField() parameters from byte[] to BytesRef.
> 2) change stringField() parameters from byte[] to String.
> I also changed the related contents while doing the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (LUCENE-8805) Parameter changes for binaryField() and stringField() in StoredFieldVisitor
[
https://issues.apache.org/jira/browse/LUCENE-8805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16844051#comment-16844051
]
Namgyu Kim commented on LUCENE-8805:
Thank you for your reply, [~jpountz].
Yes. I agree with you :D
I think it's okay to change the parameters of stringField.
However, in case of binaryField, there may be more disadvantages than
advantages.
What do you think about this, [~rcmuir]?
> Parameter changes for binaryField() and stringField() in StoredFieldVisitor
> ---
>
> Key: LUCENE-8805
> URL: https://issues.apache.org/jira/browse/LUCENE-8805
> Project: Lucene - Core
> Issue Type: Improvement
>Reporter: Namgyu Kim
>Priority: Major
> Attachments: LUCENE-8805.patch
>
>
> I wrote this patch after seeing the comments left by [~mikemccand] when
> SortingStoredFieldsConsumer class was first created.
> {code:java}
> @Override
> public void binaryField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new BR here?
> ...
> }
> @Override
> public void stringField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new String here?
> ...
> }
> {code}
> I changed two things.
> 1) change binaryField() parameters from byte[] to BytesRef.
> 2) change stringField() parameters from byte[] to String.
> I also changed the related contents while doing the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (LUCENE-8805) Parameter changes for binaryField() and stringField() in StoredFieldVisitor
[
https://issues.apache.org/jira/browse/LUCENE-8805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16843931#comment-16843931
]
Adrien Grand commented on LUCENE-8805:
--
I like how passing a String to stringField makes the API simpler. I'm less
enthusiastic about using a BytesRef instead of a byte[] for binary fields. I
could see the benefit if this allowed reusing the byte[], but otherwise it
makes the API more complicated with no benefit?
> Parameter changes for binaryField() and stringField() in StoredFieldVisitor
> ---
>
> Key: LUCENE-8805
> URL: https://issues.apache.org/jira/browse/LUCENE-8805
> Project: Lucene - Core
> Issue Type: Improvement
>Reporter: Namgyu Kim
>Priority: Major
> Attachments: LUCENE-8805.patch
>
>
> I wrote this patch after seeing the comments left by [~mikemccand] when
> SortingStoredFieldsConsumer class was first created.
> {code:java}
> @Override
> public void binaryField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new BR here?
> ...
> }
> @Override
> public void stringField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new String here?
> ...
> }
> {code}
> I changed two things.
> 1) change binaryField() parameters from byte[] to BytesRef.
> 2) change stringField() parameters from byte[] to String.
> I also changed the related contents while doing the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (LUCENE-8805) Parameter changes for binaryField() and stringField() in StoredFieldVisitor
[
https://issues.apache.org/jira/browse/LUCENE-8805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16843485#comment-16843485
]
Namgyu Kim commented on LUCENE-8805:
Thank you for your reply, and I'm sorry for late reply. [~rcmuir]
I will upload a new patch within a few days, based on your feedback.
(parameter checking, creating a TC, etc...)
> Parameter changes for binaryField() and stringField() in StoredFieldVisitor
> ---
>
> Key: LUCENE-8805
> URL: https://issues.apache.org/jira/browse/LUCENE-8805
> Project: Lucene - Core
> Issue Type: Improvement
>Reporter: Namgyu Kim
>Priority: Major
> Attachments: LUCENE-8805.patch
>
>
> I wrote this patch after seeing the comments left by [~mikemccand] when
> SortingStoredFieldsConsumer class was first created.
> {code:java}
> @Override
> public void binaryField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new BR here?
> ...
> }
> @Override
> public void stringField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new String here?
> ...
> }
> {code}
> I changed two things.
> 1) change binaryField() parameters from byte[] to BytesRef.
> 2) change stringField() parameters from byte[] to String.
> I also changed the related contents while doing the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (LUCENE-8805) Parameter changes for binaryField() and stringField() in StoredFieldVisitor
[
https://issues.apache.org/jira/browse/LUCENE-8805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16843218#comment-16843218
]
Robert Muir commented on LUCENE-8805:
-
Another idea: we may want to add some tests where we pass BytesRef in, and the
offset is nonzero. There may be some code somewhere consuming the data only
works properly when {{offset=0}}, which needs fixing. It has happened a few
times before.
> Parameter changes for binaryField() and stringField() in StoredFieldVisitor
> ---
>
> Key: LUCENE-8805
> URL: https://issues.apache.org/jira/browse/LUCENE-8805
> Project: Lucene - Core
> Issue Type: Improvement
>Reporter: Namgyu Kim
>Priority: Major
> Attachments: LUCENE-8805.patch
>
>
> I wrote this patch after seeing the comments left by [~mikemccand] when
> SortingStoredFieldsConsumer class was first created.
> {code:java}
> @Override
> public void binaryField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new BR here?
> ...
> }
> @Override
> public void stringField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new String here?
> ...
> }
> {code}
> I changed two things.
> 1) change binaryField() parameters from byte[] to BytesRef.
> 2) change stringField() parameters from byte[] to String.
> I also changed the related contents while doing the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (LUCENE-8805) Parameter changes for binaryField() and stringField() in StoredFieldVisitor
[
https://issues.apache.org/jira/browse/LUCENE-8805?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16843217#comment-16843217
]
Robert Muir commented on LUCENE-8805:
-
Should we add any safety checks? With the previous code, because we created
{{new BytesRef}} or {{new String}} there was some implicit null checks. So I am
just thinking, rather than assign the String/BytesRef directly with code such
as {{stringValue = value}} it might need {{stringValue =
Objects.requireNonNull(value)}}. Otherwise a simple user mistake might give
confusing exceptions or behavior, because it fails somewhere in low-level
codecs code.
With BytesRef as an incoming parameter its more complicated, user can also
screw up its instance variables directly and pass bogus stuff. Adding lots of
checks beyond a simple null check might be controversial. A good compromise can
be to at least add an assert to help users with their tests, e.g. {{assert
value.isValid()}}
> Parameter changes for binaryField() and stringField() in StoredFieldVisitor
> ---
>
> Key: LUCENE-8805
> URL: https://issues.apache.org/jira/browse/LUCENE-8805
> Project: Lucene - Core
> Issue Type: Improvement
>Reporter: Namgyu Kim
>Priority: Major
> Attachments: LUCENE-8805.patch
>
>
> I wrote this patch after seeing the comments left by [~mikemccand] when
> SortingStoredFieldsConsumer class was first created.
> {code:java}
> @Override
> public void binaryField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new BR here?
> ...
> }
> @Override
> public void stringField(FieldInfo fieldInfo, byte[] value) throws IOException
> {
> ...
> // TODO: can we avoid new String here?
> ...
> }
> {code}
> I changed two things.
> 1) change binaryField() parameters from byte[] to BytesRef.
> 2) change stringField() parameters from byte[] to String.
> I also changed the related contents while doing the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
