Derby
  1. Derby
  2. DERBY-4515

Document and clarify the use of DataValueDescriptor.setValue(InputStream,int)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.6.1.0
    • Fix Version/s: 10.5.3.1, 10.6.1.0
    • Component/s: JDBC
    • Labels:
      None

      Description

      The usage of the method DataValueDescriptor.setValue(InputStream stream, int length) is unclear. The intended use seems to be to pass on the known length of an input stream set from the JDBC-layer (i.e. setBinaryStream).
      There seems to be two distinct cases:

      • the logical length of the stream is known
      • the logical length of the stream is not known

      Using -1 when the length is not known seems to be an established pattern.

        Issue Links

          Activity

          Hide
          Kristian Waagan added a comment -

          Committed to trunk with revision 901648, and back-ported to the 10.5 branch with revision 901651.

          I'm closing the issue. Thank you for the feedback.

          Show
          Kristian Waagan added a comment - Committed to trunk with revision 901648, and back-ported to the 10.5 branch with revision 901651. I'm closing the issue. Thank you for the feedback.
          Hide
          Knut Anders Hatlen added a comment -

          The patch looks like an improvement to me too. +1 to commit.

          Show
          Knut Anders Hatlen added a comment - The patch looks like an improvement to me too. +1 to commit.
          Hide
          Kristian Waagan added a comment -

          Patch 1a cleans up the usage of DataValueDescriptor.setValue(InputStream,int) with the following changes:

          • DataValueDescriptor
            Added constant UNKNOWN_LOGICAL_LENGTH (has value -1).
            Improved JavaDoc for the method setValue(InputStream,int).
          • EmbedResultSet
            Used new constant to indicate unknown length.
            Removed inaccurate comment stating the length variable isn't used.
          • EmbedPreparedStatement
            Used new constant to indicate unknown length.
            Removed false comment (already wrong, but got even worse after the fix below).
            Removed unneeded variable 'intLength'.
          • SQLChar & SQLBinary
            Added mention of the new constant in the JavaDoc for the setValue method.

          Regression tests passed.
          Patch ready for review.

          An alternative to the patch, or maybe even follow-up work, is to investigate if the length argument is really needed. Most of the usage seem to be related to getting data into Derby, but there are indications that in some cases it is used when dealing with streams coming from the store as well (or maybe when passing between different parts of the system).
          Given that most of the patch is documentation and trivial changes, I think it is incremental improvement in any case.

          Show
          Kristian Waagan added a comment - Patch 1a cleans up the usage of DataValueDescriptor.setValue(InputStream,int) with the following changes: DataValueDescriptor Added constant UNKNOWN_LOGICAL_LENGTH (has value -1). Improved JavaDoc for the method setValue(InputStream,int). EmbedResultSet Used new constant to indicate unknown length. Removed inaccurate comment stating the length variable isn't used. EmbedPreparedStatement Used new constant to indicate unknown length. Removed false comment (already wrong, but got even worse after the fix below). Removed unneeded variable 'intLength'. SQLChar & SQLBinary Added mention of the new constant in the JavaDoc for the setValue method. Regression tests passed. Patch ready for review. An alternative to the patch, or maybe even follow-up work, is to investigate if the length argument is really needed. Most of the usage seem to be related to getting data into Derby, but there are indications that in some cases it is used when dealing with streams coming from the store as well (or maybe when passing between different parts of the system). Given that most of the patch is documentation and trivial changes, I think it is incremental improvement in any case.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development