Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.5.1.1
    • Fix Version/s: 10.5.1.1
    • Component/s: JDBC
    • Labels:
      None

      Description

      A set of state information is required about byte streams representing characters to be able to handle them properly.

      The basic pieces of information are:

      • is bufferable
      • is position aware
      • data offset
      • current byte position
      • current char position
      • byte length
      • char length

      Additional info:

      • encoding
      • max allowed length
      • bytes per char information

      The information is chosen with UTF8Reader in mind, and the plan is to pass a CharacterStreamDescriptor instance to the constructor to allow the reader to configure itself appropriately. The information is expected to be valid only at a specific point in time, as some of it will change as soon as the reader starts working with the underlying stream.

      1. derby-3936-4b-streamref_and_copy.diff
        13 kB
        Kristian Waagan
      2. derby-3936-4a-streamref_and_copy.diff
        13 kB
        Kristian Waagan
      3. derby-3936-3a-javadoc-1.diff
        5 kB
        Kristian Waagan
      4. derby-3936-2a-temp_fix.diff
        0.6 kB
        Kristian Waagan
      5. derby-3936-1b-CharacterStreamDescriptor.diff
        16 kB
        Kristian Waagan
      6. derby-3936-1a-CharacterStreamDescriptor.diff
        15 kB
        Kristian Waagan

        Activity

        Kristian Waagan created issue -
        Kristian Waagan made changes -
        Field Original Value New Value
        Component/s JDBC [ 11407 ]
        Affects Version/s 10.5.0.0 [ 12313010 ]
        Description A set of state information is required about byte streams representing characters to be able to handle them properly.

        The basic pieces of information are:
         - is bufferable
         - is position aware
         - data offset
         - current byte position
         - current char position
         - byte length
         - char length

        Additional info:
         - encoding
         - max allowed length
         - bytes per char information

        The information is chosen with UTF8Reader in mind, and the plan is to pass a CharacterStreamDescriptor instance to the constructor to allow the reader to configure itself appropriately. The information is expected to be valid only at a specific point in time, as some of it will change as soon as the reader starts working with the underlying stream.
        Priority Major [ 3 ] Minor [ 4 ]
        Kristian Waagan made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Kristian Waagan added a comment -

        'derby-3936-1a-CharacterStreamDescriptor.diff' is the first revision of the CharacterStreamDescriptor class.

        There is no code showing how it will be used yet, but I plan to pass it to UTF8Reader so that it can properly configure itself.
        Patch ready for review.

        What you people think of the builder pattern used?
        The reasons I tried it are to avoid a constructor with a load of arguments, avoid having a set of many constructors and a wish to keep the descriptor object immutable.

        Show
        Kristian Waagan added a comment - 'derby-3936-1a-CharacterStreamDescriptor.diff' is the first revision of the CharacterStreamDescriptor class. There is no code showing how it will be used yet, but I plan to pass it to UTF8Reader so that it can properly configure itself. Patch ready for review. What you people think of the builder pattern used? The reasons I tried it are to avoid a constructor with a load of arguments, avoid having a set of many constructors and a wish to keep the descriptor object immutable.
        Kristian Waagan made changes -
        Kristian Waagan made changes -
        Derby Info [Patch Available]
        Hide
        Knut Anders Hatlen added a comment -

        I agree that the builder pattern is cleaner than having many constructors and/or a constructor with a dozen parameters when immutability is a goal. But doesn't Builder's two-arg constructor break the pattern? Isn't it clearer to have methods for those two fields as well?

        Show
        Knut Anders Hatlen added a comment - I agree that the builder pattern is cleaner than having many constructors and/or a constructor with a dozen parameters when immutability is a goal. But doesn't Builder's two-arg constructor break the pattern? Isn't it clearer to have methods for those two fields as well?
        Hide
        Kristian Waagan added a comment -

        The reason I added a constructor to the builder is because I regarded those two pieces of information to be mandatory because of their performance implications.
        If we let them have default values of false, I agree they should follow the pattern - for instance by adding isBufferable(boolean), or maybe just isBufferable().

        There are also other details, like if the first character is at position zero or one. I chose one, and also added a notion of "before first", as a stream with a header at byte position zero is positioned before the first character.

        I don't feel to strongly on this issue, I'll be happy to comply with any wishes

        Show
        Kristian Waagan added a comment - The reason I added a constructor to the builder is because I regarded those two pieces of information to be mandatory because of their performance implications. If we let them have default values of false, I agree they should follow the pattern - for instance by adding isBufferable(boolean), or maybe just isBufferable(). There are also other details, like if the first character is at position zero or one. I chose one, and also added a notion of "before first", as a stream with a header at byte position zero is positioned before the first character. I don't feel to strongly on this issue, I'll be happy to comply with any wishes
        Hide
        Knut Anders Hatlen added a comment -

        Having default values of false and adding the methods sounds fine to me. I think
        new Builder().bufferable(true).positionAware(true).dataOffset(100).build()
        may be easier to read than
        new Builder(true, true).dataOffset(100).build()

        As to the position issue, since the class is in iapi.jdbc I guess it makes some sense to follow the JDBC convention of saying that the first character is at position 1. If it turns out that it makes the calculations more complex, it can always be changed later.

        Show
        Knut Anders Hatlen added a comment - Having default values of false and adding the methods sounds fine to me. I think new Builder().bufferable(true).positionAware(true).dataOffset(100).build() may be easier to read than new Builder(true, true).dataOffset(100).build() As to the position issue, since the class is in iapi.jdbc I guess it makes some sense to follow the JDBC convention of saying that the first character is at position 1. If it turns out that it makes the calculations more complex, it can always be changed later.
        Hide
        Kristian Waagan added a comment -

        Clearing patch available flag, a new patch is in the pipeline.

        Show
        Kristian Waagan added a comment - Clearing patch available flag, a new patch is in the pipeline.
        Kristian Waagan made changes -
        Derby Info [Patch Available]
        Hide
        Kristian Waagan added a comment -

        Attached version 1b.
        Reworked the builder according to suggestions. All parameters/settings are now considered optional.
        Validation is still being done in sane builds only.

        Show
        Kristian Waagan added a comment - Attached version 1b. Reworked the builder according to suggestions. All parameters/settings are now considered optional. Validation is still being done in sane builds only.
        Kristian Waagan made changes -
        Hide
        Knut Anders Hatlen added a comment -

        1b looks fine to me. +1 to commit.

        Show
        Knut Anders Hatlen added a comment - 1b looks fine to me. +1 to commit.
        Hide
        Kristian Waagan added a comment -

        Thanks Knut Anders.
        I committed patch 1b to trunk with revision 718936.

        Depending on how things play out, I might consider adding a reference to the stream in the descriptor object.
        I'll leave the issue open until I have a clearer view of how, and where, the descriptor object will be used.
        If anyone has opinions on keeping a reference to the described stream in the descriptor, please add a comment.

        Show
        Kristian Waagan added a comment - Thanks Knut Anders. I committed patch 1b to trunk with revision 718936. Depending on how things play out, I might consider adding a reference to the stream in the descriptor object. I'll leave the issue open until I have a clearer view of how, and where, the descriptor object will be used. If anyone has opinions on keeping a reference to the described stream in the descriptor, please add a comment.
        Kristian Waagan made changes -
        Fix Version/s 10.5.0.0 [ 12313010 ]
        Hide
        Kristian Waagan added a comment -

        Attached 'derby-3936-2a-temp_fix.diff', which is a temporary fix to include the class file(s) in derby.jar.

        Committed to trunk with revision 719217.
        Note that this is a temporary fix that should be backed out when DERBY-3934 has been fully resolved.

        Show
        Kristian Waagan added a comment - Attached 'derby-3936-2a-temp_fix.diff', which is a temporary fix to include the class file(s) in derby.jar. Committed to trunk with revision 719217. Note that this is a temporary fix that should be backed out when DERBY-3934 has been fully resolved.
        Kristian Waagan made changes -
        Attachment derby-3936-2a-temp_fix.diff [ 12394345 ]
        Hide
        Kristian Waagan added a comment -

        'derby-3936-3a-javadoc-1.diff' adds and corrects some JavaDoc in CharacterStreamDescriptor.

        Committed to trunk with revision 721200.

        Show
        Kristian Waagan added a comment - 'derby-3936-3a-javadoc-1.diff' adds and corrects some JavaDoc in CharacterStreamDescriptor. Committed to trunk with revision 721200.
        Kristian Waagan made changes -
        Attachment derby-3936-3a-javadoc-1.diff [ 12394853 ]
        Hide
        Kristian Waagan added a comment -

        'derby-3936-4a-streamref_and_copy.diff' adds the following new "features";
        o adds a stream reference to the descriptor
        o adds a method to return a PositionedStream reference to the InputStream
        o adds a method to the builder to copy the values of another builder
        o adds a few more sanity checks (sane builds only)
        o updated tests

        I think the descriptor is getting close to finished now. Optimization information described in DERBY-3907 might be added later.

        Patch ready for review.

        Show
        Kristian Waagan added a comment - 'derby-3936-4a-streamref_and_copy.diff' adds the following new "features"; o adds a stream reference to the descriptor o adds a method to return a PositionedStream reference to the InputStream o adds a method to the builder to copy the values of another builder o adds a few more sanity checks (sane builds only) o updated tests I think the descriptor is getting close to finished now. Optimization information described in DERBY-3907 might be added later. Patch ready for review.
        Kristian Waagan made changes -
        Attachment derby-3936-4a-streamref_and_copy.diff [ 12395085 ]
        Kristian Waagan made changes -
        Derby Info [Patch Available]
        Hide
        Kristian Waagan added a comment -

        Committed patch 4b to trunk with revision 722443.
        The only changes between 4a and 4b are JavaDoc.

        Show
        Kristian Waagan added a comment - Committed patch 4b to trunk with revision 722443. The only changes between 4a and 4b are JavaDoc.
        Kristian Waagan made changes -
        Derby Info [Patch Available]
        Kristian Waagan made changes -
        Attachment derby-3936-4b-streamref_and_copy.diff [ 12395089 ]
        Hide
        Kristian Waagan added a comment -

        Backed out temporary fix to get the class included in the product jars with revision 725639.

        Resolving issue, any bugs and new features will be handled under separate issues.

        Show
        Kristian Waagan added a comment - Backed out temporary fix to get the class included in the product jars with revision 725639. Resolving issue, any bugs and new features will be handled under separate issues.
        Kristian Waagan made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Kristian Waagan made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Myrna van Lunteren made changes -
        Affects Version/s 10.5.1.1 [ 12313771 ]
        Affects Version/s 10.5.0.0 [ 12313010 ]
        Fix Version/s 10.5.1.1 [ 12313771 ]
        Fix Version/s 10.5.0.0 [ 12313010 ]
        Gavin made changes -
        Workflow jira [ 12445642 ] Default workflow, editable Closed status [ 12799194 ]

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development