You are viewing a plain text version of this content. The canonical link for it is here.
Posted to solr-dev@lucene.apache.org by "Chris A. Mattmann (JIRA)" <ji...@apache.org> on 2009/12/26 19:46:29 UTC
[jira] Created: (SOLR-1688) Inner class FieldCacheSources should be
refactored into their own classes
Inner class FieldCacheSources should be refactored into their own classes
-------------------------------------------------------------------------
Key: SOLR-1688
URL: https://issues.apache.org/jira/browse/SOLR-1688
Project: Solr
Issue Type: Improvement
Components: search
Affects Versions: 1.4
Environment: indep. of env.
Reporter: Chris A. Mattmann
Fix For: 1.5
While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794714#action_12794714 ]
Yonik Seeley commented on SOLR-1688:
------------------------------------
IMO, most of these should remain implementation details (i.e. not public)... they weren't thought out in sufficient detail to support as public classes (and there has been little reason to do so).
If we need StrValueSource to be public for another issue, then we should limit the change to that.
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12843924#action_12843924 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
Any comments on this guys? Compromise? Standoff? White flag? :P
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794747#action_12794747 ]
Yonik Seeley commented on SOLR-1688:
------------------------------------
bq. Then why isn't ValueSource and FieldCacheSource in the o.a.solr.schema package?
Historical - ValueSource came from function query... but it's become(ing) more useful and applicable.
Rhetorical question: should all implementations of Map be in the java.util package?
bq. And more so, why is it OK for some and not OK for others
Think of it this way - some FieldType's may implement their getValueSource with a publicly defined ValueSource and others may not. It's not the case that for a given FooFieldType that there should be a publicly usable FooValueSource. There should never be any guarantee that a specific FieldType use a specific implementation of a ValueSource. The only guarantee should be that it work. And if that's the case, one should ask why all these value sources should be public?
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794748#action_12794748 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
{quote}
Rhetorical question: should all implementations of Map be in the java.util package?
{quote}
If they are defined as a "core" data structure part of the JDK, then I would say yes. It's not as black and white of a line as you make it out to be. You can have SOLR be entirely a plugin-based system, with nothing but configuration inside of SVN, or you can have every piece of code that interacts with SOLR be inside the SOLR SVN. Neither solution will work, you have to strike a balance. The same applies for code organization and using absolutes or extremes doesn't really illustrate much.
{quote}
Think of it this way - some FieldType's may implement their getValueSource with a publicly defined ValueSource and others may not.
{quote}
This makes sense so long as there is a reason. Can you tell me the reason that e.g., StrFieldSource exists inside of StrField while DoubleFieldSource exists outside of DoubleField? Or why the other 4 or 5 FieldSources that are defined inside of their own java file exist there, while the other 4 or 5 defined inside of the FieldType's java file exist there? What's the litmus test? I'm not sure I see one, which is why I created this issue. And let's be clear. Consistency, code style, maintainability _are_ important issues and following a pattern in one situation while not following it in another (similar or same) situation is not a great example of style or consistency.
{quote}It's not the case that for a given FooFieldType that there should be a publicly usable FooValueSource. There should never be any guarantee that a specific FieldType use a specific implementation of a ValueSource. The only guarantee should be that it work. {quote}
The patch I put up still allows everything to work and doesn't change this requirement.
{quote}
And if that's the case, one should ask why all these value sources should be public?
{quote}
Because it's more consistent, and thus, more maintainable. Because when you tell someone to modify one of the core FieldSources or ValueSources, they know where to look instead of, "oh is this one inside of a class inside of o.a.solr.schema, or is this one in the o.a.solr.search.function package"? You can argue that it doesn't take much time to make that determination, which I'll give you, but I'll argue back it isn't consistent. If there's not a good reason to make them all live in search.function or live inside of the FieldTypes in schema, and it's really a case-by-case reason, then I'd like to hear some of those case-by-cases.
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Shalin Shekhar Mangar (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794664#action_12794664 ]
Shalin Shekhar Mangar commented on SOLR-1688:
---------------------------------------------
Chris, isn't referring to it as a ValueSource instance enough for SOLR-1586?
{quote}
What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource).
{quote}
That is not really a problem. The field types are always loaded by Solr so whether they are an inner class or independent does not matter too much.
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SOLR-1688) Inner class FieldCacheSources should be
refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Chris A. Mattmann updated SOLR-1688:
------------------------------------
Attachment: SOLR-1688.Mattmann.122609.patch.txt
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794729#action_12794729 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
{quote}
But at this point... why mess with it? "consistency"?
{quote}
Yes, consistency. And the fact that applying this patch doesn't break anything, it just cleans it up. With respect to "why mess with it", I guess my question is: why not?
Furthermore, like I said above, the placement of ValueSource and FieldCacheSource in the o.a.solr.search.function package suggests that these classes and their children likely belong in that package rather than o.a.solr.schema, or furthermore that some refactoring is necessary. This is an initial step towards that refactoring.
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794727#action_12794727 ]
Yonik Seeley commented on SOLR-1688:
------------------------------------
bq. The troubling part is that some of these classes, e.g., DoubleFieldSource, FloatFieldSource, are defined as outer, public classes
Perhaps they should not have been public either.
But at this point... why mess with it? "consistency"? People shouldn't be grabbing or referencing specific implementations of ValueSource of field types - that's why the field type as a getValueSource(). For the rare expert level users creating their own FieldType implementations (and that should be a rare), it shouldn't be an issue.
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Shalin Shekhar Mangar (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794782#action_12794782 ]
Shalin Shekhar Mangar commented on SOLR-1688:
---------------------------------------------
{quote}
IMO, most of these should remain implementation details (i.e. not public)... they weren't thought out in sufficient detail to support as public classes (and there has been little reason to do so).
If we need StrValueSource to be public for another issue, then we should limit the change to that.
{quote}
+1
As they say, lets not fix what ain't broken.
{quote}
If they are defined as a "core" data structure part of the JDK, then I would say yes. It's not as black and white of a line as you make it out to be. You can have SOLR be entirely a plugin-based system, with nothing but configuration inside of SVN, or you can have every piece of code that interacts with SOLR be inside the SOLR SVN. Neither solution will work, you have to strike a balance. The same applies for code organization and using absolutes or extremes doesn't really illustrate much.
{quote}
Chris, we are striving for balance and we are OK with the change to StrFieldSource. In this particular case, you seem to be pushing towards extremes in the name of consistency.
{quote}
Can you tell me the reason that e.g., StrFieldSource exists inside of StrField while DoubleFieldSource exists outside of DoubleField? Or why the other 4 or 5 FieldSources that are defined inside of their own java file exist there, while the other 4 or 5 defined inside of the FieldType's java file exist there? What's the litmus test?
{quote}
It is not a public API and I guess that at the time it was written, there was no reason to make it one. It was convenient or a matter of personal style or most likely a random choice. There is no litmus test and there does not have to be one.
{quote}
Because it's more consistent, and thus, more maintainable.
{quote}
Actually it is the other way round. Once you make it public, it is harder to maintain. All changes should then be backward compatible as far as possible. The bottom line is that making all of them public is not needed. Your opinion is that it is broken because it is not consistent. My opinion is that it is OK and it does not matter. We shouldn't lean towards making something a public API in the name of consistency.
{quote}
Because when you tell someone to modify one of the core FieldSources or ValueSources, they know where to look instead of, "oh is this one inside of a class inside of o.a.solr.schema, or is this one in the o.a.solr.search.function package"?
{quote}
Most IDEs have a way to goto the source of a particular class, otherwise there is grep. The point is that many (most?) of these classes don't need to be modified unless in very rare cases. If it becomes a common practice to modify them, then there is probably something wrong with our APIs and we need to re-think them.
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794688#action_12794688 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
bq. Chris, isn't referring to it as a ValueSource instance enough for SOLR-1586?
Unfortunately, no. Grant's last patch for SOLR-1586 created a StrFieldSource, which isn't visible outside of its class.
bq. That is not really a problem. The field types are always loaded by Solr so whether they are an inner class or independent does not matter too much.
It's not a problem as much as it is inconsistent. Some FieldCacheSources are defined inline in their associated FieldType class, some are defined outside of it, for no rhyme or reason. This patch makes them all consistently defined in their own separate class, making it easier for extension, use and inheritance.
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (SOLR-1688) Inner class
FieldCacheSources should be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794833#action_12794833 ]
Chris A. Mattmann edited comment on SOLR-1688 at 12/28/09 4:41 PM:
-------------------------------------------------------------------
bq. Chris, we are striving for balance and we are OK with the change to StrFieldSource. In this particular case, you seem to be pushing towards extremes in the name of consistency.
Yes, I am, because it's better to be extremely consistent when you are developing a code base that's seen by thousands of developers around the world.
bq. It is not a public API and I guess that at the time it was written, there was no reason to make it one. It was convenient or a matter of personal style or most likely a random choice. There is no litmus test and there does not have to be one.
Um, actually it _is_ a public API. ValueSource and FieldCacheSource are both public classes within the o.a.solr.search.function package. Anyone can write them. And if you're agreeing it was a random choice, why not turn it into a principled decision?
{quote}
Actually it is the other way round. Once you make it public, it is harder to maintain. All changes should then be backward compatible as far as possible. The bottom line is that making all of them public is not needed. Your opinion is that it is broken because it is not consistent. My opinion is that it is OK and it does not matter. We shouldn't lean towards making something a public API in the name of consistency.
{quote}
Actually it's not the other way around. Public APIs aren't harder to maintain. It's kind of a matter of debate. It's also mixing levels of granularity because public from an external (client) to SOLR server interface perspective is different from public at the code, library or function level as part of SOLR. Additionally, changes being backwards compatible is one of the many non-functional concerns -- there are others. Ease of use, portability, maintainability, understandability, etc.You can't have a blanket policy for maximizing one, without affecting the others.
Let's be clear. I'm not advocating that something be made a public API that _isn't_ already public. ValueSource and FieldCacheSource are public _classes_. And note the difference between _class_ and _API_. ValueSource and FieldCacheSource are not _API_ s, they are Java classes (different levels of granularity).
Because of this class-level decision, the codebase itself contains inconsistent use of these 2 classes:
* as separate Java classes defined in a FieldType's Java file
* as Java classes defined in their own Java file that lives within o.a.solr.search.function
Also let's be clear also -- I never said "broken", I said "inconsistent". If what you're saying is that you as a SOLR committer don't care about inconsistency, then I'm sorry to hear that.
{quote}
Most IDEs have a way to goto the source of a particular class, otherwise there is grep. The point is that many (most?) of these classes don't need to be modified unless in very rare cases. If it becomes a common practice to modify them, then there is probably something wrong with our APIs and we need to re-think them.
{quote}
If you're advocating using grep or using an IDE's search functionality as opposed to just understanding where code should be located based on principled architectural design, then again, I'm sorry to hear that. Especially when the principled design comes at 0 cost. The patch I attached didn't break anything, didn't change any APIs, furthermore, didn't change any Java classes, didn't change anything. All I did was re-organize the code to be a bit more principled in its organization. I can explain why I would put all the code in o.a.solr.search.function. Can you explain why it's not?
was (Author: chrismattmann):
bq. Chris, we are striving for balance and we are OK with the change to StrFieldSource. In this particular case, you seem to be pushing towards extremes in the name of consistency.
Yes, I am, because it's better to be extremely consistent when you are developing a code base that's seen by thousands of developers around the world.
bq. It is not a public API and I guess that at the time it was written, there was no reason to make it one. It was convenient or a matter of personal style or most likely a random choice. There is no litmus test and there does not have to be one.
Um, actually it _is_ a public API. ValueSource and FieldCacheSource are both public classes within the o.a.solr.search.function package. Anyone can write them. And if you're agreeing it was a random choice, why not turn it into a principled decision?
{quote}
Actually it is the other way round. Once you make it public, it is harder to maintain. All changes should then be backward compatible as far as possible. The bottom line is that making all of them public is not needed. Your opinion is that it is broken because it is not consistent. My opinion is that it is OK and it does not matter. We shouldn't lean towards making something a public API in the name of consistency.
{quote}
Actually it's not the other way around. Public APIs aren't harder to maintain. It's kind of a matter of debate. It's also mixing levels of granularity because public from an external (client) to SOLR server interface perspective is different from public at the code, library or function level as part of SOLR. Additionally, changes being backwards compatible is one of the many non-functional concerns -- there are others. Ease of use, portability, maintainability, understandability, etc.You can't have a blanket policy for maximizing one, without affecting the others.
Let's be clear. I'm not advocating that something be made a public API that _isn't_ already public. ValueSource and FieldCacheSource are public _classes_. And note the difference between _class_ and _API_. ValueSource and FieldCacheSource are not _API_s, they are Java classes (different levels of granularity).
Because of this class-level decision, the codebase itself contains inconsistent use of these 2 classes:
* as separate Java classes defined in a FieldType's Java file
* as Java classes defined in their own Java file that lives within o.a.solr.search.function
Also let's be clear also -- I never said "broken", I said "inconsistent". If what you're saying is that you as a SOLR committer don't care about inconsistency, then I'm sorry to hear that.
{quote}
Most IDEs have a way to goto the source of a particular class, otherwise there is grep. The point is that many (most?) of these classes don't need to be modified unless in very rare cases. If it becomes a common practice to modify them, then there is probably something wrong with our APIs and we need to re-think them.
{quote}
If you're advocating using grep or using an IDE's search functionality as opposed to just understanding where code should be located based on principled architectural design, then again, I'm sorry to hear that. Especially when the principled design comes at 0 cost. The patch I attached didn't break anything, didn't change any APIs, furthermore, didn't change any Java classes, didn't change anything. All I did was re-organize the code to be a bit more principled in its organization. I can explain why I would put all the code in o.a.solr.search.function. Can you explain why it's not?
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794734#action_12794734 ]
Yonik Seeley commented on SOLR-1688:
------------------------------------
I'm against making all ValueSource classes public. Some of them will clearly be implementations very specific to a FieldType, and those implementation details shouldn't be exposed unless needed.
bq. the placement of ValueSource and FieldCacheSource in the o.a.solr.search.function package suggests that these classes and their children likely belong in that package
No, the implementation of FieldType.getValueSource() is up to that FieldType and it can make perfect sense to be co-located (esp if it's an implementation detail of the FieldType).
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794737#action_12794737 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
{quote}
I'm against making all ValueSource classes public. Some of them will clearly be implementations very specific to a FieldType, and those implementation details shouldn't be exposed unless needed.
{quote}
I don't get this especially when you yourself said most of these classes don't really have detailed implementations (which I can corroborate when I put the patch together for this issue looking at the code). Also, there is at least one other community request to do this, SOLR-1689, albeit for a few select ValueSources.
{quote}
No, the implementation of FieldType.getValueSource() is up to that FieldType and it can make perfect sense to be co-located (esp if it's an implementation detail of the FieldType).
{quote}
Then why isn't ValueSource and FieldCacheSource in the o.a.solr.schema package? And more so, why is it OK for some and not OK for others, especially when none of the ValueSource implementation classes is greater than a few hundred lines of code, if that?
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794722#action_12794722 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
Hi Yonik:
It wasn't a huge deal for me regarding public or private for the FieldCacheSources. I had some weird Eclipse error that pointed out that StrFieldSource was a class defined inside of another java file (originally I thought it was an inner class, but later found out it wasn't) as part of SOLR-1586. That led me to looking at this issue, and like I said the troubling part isn't that it's an inner class, versus an outer class (although outer classes are certainly cleaner for anything that will be used outside of the package). The troubling part is that some of these classes, e.g., DoubleFieldSource, FloatFieldSource, are defined as outer, public classes in an entirely different package (o.a.solr.search.function), whereas some of them (e.g., StrFieldSource, SortableDoubleFieldSource) are defined as outer classes inside of their FieldType constituents. That's inconsistent.
Furthermore, the base class, FieldCacheSource, and ultimately its parent, ValueSource, are both intrinsically tied to the o.a.solr.search.function package, and are both public outer classes defined in their own java file(s). This is also inconsistent.
The patch I attached associates all FieldCacheSources in the o.a.solr.search.function package (where the rest of them are defined and where their parents are also defined) and IMO fixes up these inconsistencies.
Cheers,
Chris
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12794833#action_12794833 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
bq. Chris, we are striving for balance and we are OK with the change to StrFieldSource. In this particular case, you seem to be pushing towards extremes in the name of consistency.
Yes, I am, because it's better to be extremely consistent when you are developing a code base that's seen by thousands of developers around the world.
bq. It is not a public API and I guess that at the time it was written, there was no reason to make it one. It was convenient or a matter of personal style or most likely a random choice. There is no litmus test and there does not have to be one.
Um, actually it _is_ a public API. ValueSource and FieldCacheSource are both public classes within the o.a.solr.search.function package. Anyone can write them. And if you're agreeing it was a random choice, why not turn it into a principled decision?
{quote}
Actually it is the other way round. Once you make it public, it is harder to maintain. All changes should then be backward compatible as far as possible. The bottom line is that making all of them public is not needed. Your opinion is that it is broken because it is not consistent. My opinion is that it is OK and it does not matter. We shouldn't lean towards making something a public API in the name of consistency.
{quote}
Actually it's not the other way around. Public APIs aren't harder to maintain. It's kind of a matter of debate. It's also mixing levels of granularity because public from an external (client) to SOLR server interface perspective is different from public at the code, library or function level as part of SOLR. Additionally, changes being backwards compatible is one of the many non-functional concerns -- there are others. Ease of use, portability, maintainability, understandability, etc.You can't have a blanket policy for maximizing one, without affecting the others.
Let's be clear. I'm not advocating that something be made a public API that _isn't_ already public. ValueSource and FieldCacheSource are public _classes_. And note the difference between _class_ and _API_. ValueSource and FieldCacheSource are not _API_s, they are Java classes (different levels of granularity).
Because of this class-level decision, the codebase itself contains inconsistent use of these 2 classes:
* as separate Java classes defined in a FieldType's Java file
* as Java classes defined in their own Java file that lives within o.a.solr.search.function
Also let's be clear also -- I never said "broken", I said "inconsistent". If what you're saying is that you as a SOLR committer don't care about inconsistency, then I'm sorry to hear that.
{quote}
Most IDEs have a way to goto the source of a particular class, otherwise there is grep. The point is that many (most?) of these classes don't need to be modified unless in very rare cases. If it becomes a common practice to modify them, then there is probably something wrong with our APIs and we need to re-think them.
{quote}
If you're advocating using grep or using an IDE's search functionality as opposed to just understanding where code should be located based on principled architectural design, then again, I'm sorry to hear that. Especially when the principled design comes at 0 cost. The patch I attached didn't break anything, didn't change any APIs, furthermore, didn't change any Java classes, didn't change anything. All I did was re-organize the code to be a bit more principled in its organization. I can explain why I would put all the code in o.a.solr.search.function. Can you explain why it's not?
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-1688) Inner class FieldCacheSources should
be refactored into their own classes
Posted by "Chris A. Mattmann (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-1688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12799618#action_12799618 ]
Chris A. Mattmann commented on SOLR-1688:
-----------------------------------------
Hi All,
Yonik, what do you think on this one? The patch I put up at least follows a single policy and has rationale rather than the existing code which does not (see comments above). What's the compromise here? Let's get this cleaned up! :)
Cheers,
Chris
> Inner class FieldCacheSources should be refactored into their own classes
> -------------------------------------------------------------------------
>
> Key: SOLR-1688
> URL: https://issues.apache.org/jira/browse/SOLR-1688
> Project: Solr
> Issue Type: Improvement
> Components: search
> Affects Versions: 1.4
> Environment: indep. of env.
> Reporter: Chris A. Mattmann
> Fix For: 1.5
>
> Attachments: SOLR-1688.Mattmann.122609.patch.txt
>
>
> While working on SOLR-1586 I noticed that outside of class level access (or package level), you can't really reference FieldCacheSources that are defined inside of their FieldType constituents (e.g., in the case of StrFieldSource as defined in StrField). What's more troubling is that the FieldType/FieldCacheSources are defined in an inconsistent fashion: some are done as inner classes, e.g., StrFieldSource and SortableFloatFieldSource, while others are defined as individual classes (e.g., FloatFIeldSource). This patch will make them all consistent and define each FieldCacheSource as an outside class, present in o.a.solr.search.function.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.