Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None
    1. SOLR-9721.patch
      22 kB
      Noble Paul
    2. SOLR-9721.patch
      26 kB
      Noble Paul

      Issue Links

        Activity

        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        It seems like it would be cleaner / easier (and avoid adding other endpoints) to just do it like /select. Have "wt" control the desired output format.

        Also, the new javabin.json format stuff still seems like overkill. It seems like it would be much less code to map it on the client side as desired.
        I think we're only talking about SolrDocument and SolrDocumentList? Those can both be mapped by the client if it cares and we don't need all these new classes.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - It seems like it would be cleaner / easier (and avoid adding other endpoints) to just do it like /select. Have "wt" control the desired output format. Also, the new javabin.json format stuff still seems like overkill. It seems like it would be much less code to map it on the client side as desired. I think we're only talking about SolrDocument and SolrDocumentList? Those can both be mapped by the client if it cares and we don't need all these new classes.
        Hide
        noble.paul Noble Paul added a comment - - edited

        My preference is to have the type information preserved for primitive types and get rid of all the extra container objects and just have map and list

        In the future, I would like to limit the types to far fewer than what we have today. it makes no sense to a user/developer when he sees a Map in JSON and the payload may contain Map/NamedList/SimpleOrderedMap/SolrDocument etc. it's not a good idea to convert any of these objects to Map just in time at client side , it is just leads to unnecessary object creation

        Show
        noble.paul Noble Paul added a comment - - edited My preference is to have the type information preserved for primitive types and get rid of all the extra container objects and just have map and list In the future, I would like to limit the types to far fewer than what we have today. it makes no sense to a user/developer when he sees a Map in JSON and the payload may contain Map/NamedList/SimpleOrderedMap/SolrDocument etc. it's not a good idea to convert any of these objects to Map just in time at client side , it is just leads to unnecessary object creation
        Hide
        yseeley@gmail.com Yonik Seeley added a comment - - edited

        I agree there is value to presenting standard containers only (esp for libraries that want to re-encode as JSON or something).
        But I think from an implementation point of view, the simplest way by far is to have the decoder (optionally) do it. No extra classes necessary.

        We should also avoid conflating this with "JSON". Using Map for NamedList, SolrDocument and SolrDocumentList does not mean that JSON will be able to represent everything that is sent over binary... it still can't (dates get transformed to a string, +-Infinity can't be represented, etc).

        it's not a good idea to convert any of these objects to Map just in time at client side , it is just leads to unnecessary object creation

        No it doesn't... that's an implementation detail.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - - edited I agree there is value to presenting standard containers only (esp for libraries that want to re-encode as JSON or something). But I think from an implementation point of view, the simplest way by far is to have the decoder (optionally) do it. No extra classes necessary. We should also avoid conflating this with "JSON". Using Map for NamedList, SolrDocument and SolrDocumentList does not mean that JSON will be able to represent everything that is sent over binary... it still can't (dates get transformed to a string, +-Infinity can't be represented, etc). it's not a good idea to convert any of these objects to Map just in time at client side , it is just leads to unnecessary object creation No it doesn't... that's an implementation detail.
        Hide
        noble.paul Noble Paul added a comment -

        The extra class is pretty simple and it reduces the actual cost of new Object creation at client side. It is not a lot of code. So, it's worth it

        Show
        noble.paul Noble Paul added a comment - The extra class is pretty simple and it reduces the actual cost of new Object creation at client side. It is not a lot of code. So, it's worth it
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        You're creating another response writer (JsonJavabinWriter) that extends JSONWriter. That's going to be more to maintain, plus it's confusing.
        Just make the decoder optionally create a Map when it sees a SolrDocument (for example). No need to create any new classes (or endpoints) on the server side at all!

        Show
        yseeley@gmail.com Yonik Seeley added a comment - You're creating another response writer (JsonJavabinWriter) that extends JSONWriter. That's going to be more to maintain, plus it's confusing. Just make the decoder optionally create a Map when it sees a SolrDocument (for example). No need to create any new classes (or endpoints) on the server side at all!
        Hide
        noble.paul Noble Paul added a comment - - edited

        I'm all for preserving all primitives (like date, int, float etc)

        No it doesn't... that's an implementation detail.

        When a user/developer invokes Map<String,Object>next() the code internally has to create the maps. The entire Streaming code is written with this API

        Show
        noble.paul Noble Paul added a comment - - edited I'm all for preserving all primitives (like date, int, float etc) No it doesn't... that's an implementation detail. When a user/developer invokes Map<String,Object>next() the code internally has to create the maps. The entire Streaming code is written with this API
        Hide
        noble.paul Noble Paul added a comment -

        Actually, I tried doing this. It was extremely hard and much more complex to make it work with the streaming code. It breaks at so many places

        That's going to be more to maintain, plus it's confusing.

        I wouldn't have wanted to go down that path. But , looking at JavabinCodec, it looks like a kitchen sink and it begs to be replaced with something saner. It's much more complex than it should be.

        I would like to know what is useful in using NamedList/SimpleOrderedmap/NamedList/SolrDocument compared to what JSON does today. JSON output looks much cleaner and the javabin looks awkward and confusing. There is a lot of value in having a 1:1 mapping between binary/json

        Show
        noble.paul Noble Paul added a comment - Actually, I tried doing this. It was extremely hard and much more complex to make it work with the streaming code. It breaks at so many places That's going to be more to maintain, plus it's confusing. I wouldn't have wanted to go down that path. But , looking at JavabinCodec, it looks like a kitchen sink and it begs to be replaced with something saner. It's much more complex than it should be. I would like to know what is useful in using NamedList/SimpleOrderedmap/NamedList/SolrDocument compared to what JSON does today. JSON output looks much cleaner and the javabin looks awkward and confusing. There is a lot of value in having a 1:1 mapping between binary/json
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        It was extremely hard and much more complex to make it work with the streaming code.

        Ah... I was coming from the perspective of a normal client that would say "deserialize this into an object hierarchy for me".
        I think this is the crux of the confusion... I think you're coming at this from "how do I replace JSON with binary" in the streaming API.
        Do you have a pointer into the streaming code that causes issues? I looked at SortingResponseWriter and it looks like it does the JSON encoding itself, so this patch won't affect that?

        But , looking at JavabinCodec, it looks like a kitchen sink and it begs to be replaced with something saner.

        This patch doesn't seem to do that though... (replace JavaBinCodec), it only adds to that complexity of the code.

        I would like to know what is useful in using NamedList/SimpleOrderedmap/NamedList/SolrDocument compared to what JSON does today.

        That seems like a different discussion... I already agreed "there is value to presenting standard containers only". This patch doesn't remove any of those things from the code base anyway. We're talking about the mechanism by which one can get standard java.util containers.

        There is a lot of value in having a 1:1 mapping between binary/json

        We just got done talking about the fact that it won't actually be a 1:1 mapping because binary can represent things that JSON can't. Round-tripping will fail.

        And really, if we're looking at a new binary format, there are a lot of things I'd like to clean up...

        Show
        yseeley@gmail.com Yonik Seeley added a comment - It was extremely hard and much more complex to make it work with the streaming code. Ah... I was coming from the perspective of a normal client that would say "deserialize this into an object hierarchy for me". I think this is the crux of the confusion... I think you're coming at this from "how do I replace JSON with binary" in the streaming API. Do you have a pointer into the streaming code that causes issues? I looked at SortingResponseWriter and it looks like it does the JSON encoding itself, so this patch won't affect that? But , looking at JavabinCodec, it looks like a kitchen sink and it begs to be replaced with something saner. This patch doesn't seem to do that though... (replace JavaBinCodec), it only adds to that complexity of the code. I would like to know what is useful in using NamedList/SimpleOrderedmap/NamedList/SolrDocument compared to what JSON does today. That seems like a different discussion... I already agreed "there is value to presenting standard containers only". This patch doesn't remove any of those things from the code base anyway. We're talking about the mechanism by which one can get standard java.util containers. There is a lot of value in having a 1:1 mapping between binary/json We just got done talking about the fact that it won't actually be a 1:1 mapping because binary can represent things that JSON can't. Round-tripping will fail. And really, if we're looking at a new binary format, there are a lot of things I'd like to clean up...
        Hide
        noble.paul Noble Paul added a comment -

        I started with writing a shim parser which converts types on the fly (converts javabin object types to json type Objects and primitives). The complexity came from the fact that the parser has to be a streaming pull parser to achieve the efficiency. I could not simply override certain methods on javabinCodec . It was proving to be much harder than I initially thought. However, I shall take another stab at that.

        Show
        noble.paul Noble Paul added a comment - I started with writing a shim parser which converts types on the fly (converts javabin object types to json type Objects and primitives). The complexity came from the fact that the parser has to be a streaming pull parser to achieve the efficiency. I could not simply override certain methods on javabinCodec . It was proving to be much harder than I initially thought. However, I shall take another stab at that.
        Hide
        noble.paul Noble Paul added a comment -

        Yonik Seeley The parsing is done with a shim parser

        Show
        noble.paul Noble Paul added a comment - Yonik Seeley The parsing is done with a shim parser
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 41d7b245749bb02ed46a5fed7c4ee3b7b58e8266 in lucene-solr's branch refs/heads/master from Noble Paul
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=41d7b24 ]

        SOLR-9721: javabin Tuple parser for streaming and other end points

        Show
        jira-bot ASF subversion and git services added a comment - Commit 41d7b245749bb02ed46a5fed7c4ee3b7b58e8266 in lucene-solr's branch refs/heads/master from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=41d7b24 ] SOLR-9721 : javabin Tuple parser for streaming and other end points
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 28fd42bf4ab47c130d814e1abd3e318f29ea66d7 in lucene-solr's branch refs/heads/branch_6x from Noble Paul
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=28fd42b ]

        SOLR-9721: javabin Tuple parser for streaming and other end points

        Show
        jira-bot ASF subversion and git services added a comment - Commit 28fd42bf4ab47c130d814e1abd3e318f29ea66d7 in lucene-solr's branch refs/heads/branch_6x from Noble Paul [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=28fd42b ] SOLR-9721 : javabin Tuple parser for streaming and other end points

          People

          • Assignee:
            Unassigned
            Reporter:
            noble.paul Noble Paul
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development