Uploaded image for project: 'Apache Drill'
  1. Apache Drill
  2. DRILL-2932

Error text reported via System.out.println rather than thrown SQLException's message

Details

    • Bug
    • Status: Resolved
    • Critical
    • Resolution: Fixed
    • None
    • 1.0.0
    • Client - JDBC
    • None

    Description

      There is a call to System.out.println(...) in DrillResultSetImpl.ResultListener.submissionFailed(...):

      System.out.println("Query failed: " + ex.getMessage());

      (That submissionFailed(...) is part of the implementation of Statement.execute...(...) methods and ResultSet.)

      In SQLLine, this causes the exception message, which currently doesn't show up otherwise in SQLLine, to show up when running SQLLine interactively.

      However, writing that to System.out. is completely inappropriate.

      JDBC specifies that implementations throw SQLExceptions to report errors--implementations should not be unilaterally be deciding to write to stdout--or stderr.
       

      Additionally, the text intended to reach the user is not copied into the message of the SQLException that is thrown to the JDBC client.

      Attachments

        1. DRILL-2932.2.patch.txt
          28 kB
          Daniel Barclay
        2. DRILL-2932.4.patch.txt
          28 kB
          Daniel Barclay

        Issue Links

          Activity

            dsbos Daniel Barclay added a comment - Review at: https://reviews.apache.org/r/33779/ .
            dsbos Daniel Barclay added a comment - - edited

            Patch commit message:

            DRILL-2932: Fix: Error reported via System.out rather than exception message

            Main:

            • Removed the System.out.println(...) from submissionFailed(...).
            • Put UserException's message text in SQLException's message (didn't just wrap).
            • Added undoing of extra wrapping by AvaticaStatement.execute...(...).
            • Created unit test for execute...(...) exceptions.
            • Refined related exception handling (handled cases separately, narrowed throws declarations and catches).

            JDBC cleanup:

            • Renamed ex -> executionFailureException
            • Added getNext() method doc. comment.
            • Removed some obsolete alignment spaces.

            Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):

            • DrillCursor;
            • MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
            • TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
            dsbos Daniel Barclay added a comment - - edited Patch commit message: DRILL-2932 : Fix: Error reported via System.out rather than exception message Main: Removed the System.out.println(...) from submissionFailed(...). Put UserException's message text in SQLException's message (didn't just wrap). Added undoing of extra wrapping by AvaticaStatement.execute...(...). Created unit test for execute...(...) exceptions. Refined related exception handling (handled cases separately, narrowed throws declarations and catches). JDBC cleanup: Renamed ex -> executionFailureException Added getNext() method doc. comment. Removed some obsolete alignment spaces. Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException): DrillCursor; MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch; TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
            dsbos Daniel Barclay added a comment -

            Rebased re DRILL-2884 conflict.

            dsbos Daniel Barclay added a comment - Rebased re DRILL-2884 conflict.

            People

              dsbos Daniel Barclay
              dsbos Daniel Barclay
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: