[PECOBLR-384] Add implementation for fetching statement status for async execution#824
Conversation
| * the methods defined in the interface. It is used to represent the status of a SQL statement | ||
| * execution in Databricks. | ||
| */ | ||
| class StatementStatus implements IStatementStatus { |
There was a problem hiding this comment.
can we rename this class to avoid confusion and FQN everywhere with com.databricks.jdbc.model.core.StatementStatus
| @@ -0,0 +1,24 @@ | |||
| package com.databricks.jdbc.api; | |||
|
|
|||
| public interface IStatementStatus { | |||
There was a problem hiding this comment.
do we need this interface? seems like there cannot be any other implementation other than the one below?
There was a problem hiding this comment.
It is more for hiding impl details from the client. We will only document interface methods.
jayantsing-db
left a comment
jayantsing-db
left a comment
There was a problem hiding this comment.
lgtm (barring @vikrantpuppala comment). some minor comments.
| * @return The current {@link StatementStatus} of the statement @Deprecated Use {@link | ||
| * #getExecutionStatus()} instead. |
There was a problem hiding this comment.
nice. how does this tag work? When the users try to use, does it display the warning?
There was a problem hiding this comment.
Yes, it will display compile warnings as well as javadoc warning
| * | ||
| * @return the current state of the statement execution | ||
| */ | ||
| StatementState getState(); |
There was a problem hiding this comment.
nit: if we decide to keep the interface, can we rename this method to getStatementState? that way looks more consistent with getSqlState
| @@ -0,0 +1,20 @@ | |||
| package com.databricks.jdbc.api; | |||
There was a problem hiding this comment.
should we move this to com.databricks.jdbc.common?
There was a problem hiding this comment.
This is public facing and will be documented. Don't want to move public facing to common.
| PENDING, | ||
| // The statement is currently executing. | ||
| RUNNING, | ||
| // The statement has completed successfully. | ||
| SUCCEEDED, | ||
| // The statement has completed with an error. | ||
| FAILED, | ||
| // The statement has been closed and is no longer available. | ||
| CLOSED, | ||
| // The statement has been cancelled by the user. | ||
| ABORTED; |
There was a problem hiding this comment.
nit: i think these are same for both thrift and sql execution but just confirming?
| // Constructor for SEA result set | ||
| public DatabricksResultSet( | ||
| StatementStatus statementStatus, | ||
| com.databricks.jdbc.model.core.StatementStatus statementStatus, |
There was a problem hiding this comment.
nit: maybe keep the import com.databricks.jdbc.model.core.StatementStatus to keep it concise?
| @@ -0,0 +1,20 @@ | |||
| package com.databricks.jdbc.api; | |||
There was a problem hiding this comment.
should we move to com.databricks.jdbc.common?
There was a problem hiding this comment.
Wanted to keep public facing and public documented interfaces and enums in a single place.
| return state; | ||
| } | ||
|
|
||
| com.databricks.jdbc.model.core.StatementStatus getSdkStatus() { |
There was a problem hiding this comment.
I guess this is for backward compatibility?
3 participants
Description
Changes contain:
The documentation will be added for the new interface.
Testing
Updated unit tests
Additional Notes to the Reviewer