Conversation
… will be only for getters and adding the test
…lVariant # Conflicts: # src/main/java/com/microsoft/sqlserver/jdbc/PLPInputStream.java resolved conflicts
Sql_Variant initial development
Java offers two kinds of multi line comments /** and /*. The first is called Javadoc and ends up in generated API documentation and is displayed by IDEs. The second is a multi line comment that does not show up. There are several places there /* instead of /** is used for API documentation. [1] https://stackoverflow.com/questions/29815636/and-in-java-comments [2] http://javadude.com/articles/comments.html
Usage of the Java Enum feature can be improved by the driver. This commit includes the following changes: - use ordinal where possible - make enum fields final where possible
All tests are passing, merging for next major release. @brettwooldridge, now you can start a PR against dev :-)
…ching Revert "Metadata caching - Parsed SQL and prepared statement handle caching"
…entation Use Javadoc for API documentation
Improve enum usage
UTC is an enum with one constant and a static variable. It has the
following comment
> The enum type delays initialization until first use.
This is demonstrably wrong.
The static variable is initialized when the class is initialized. If we
look at the decompiled static initializer of UTC we see that.
```
// access flags 0x8
static <clinit>()V
L0
LINENUMBER 522 L0
NEW com/microsoft/sqlserver/jdbc/UTC
DUP
LDC "INSTANCE"
ICONST_0
INVOKESPECIAL com/microsoft/sqlserver/jdbc/UTC.<init> (Ljava/lang/String;I)V
PUTSTATIC com/microsoft/sqlserver/jdbc/UTC.INSTANCE : Lcom/microsoft/sqlserver/jdbc/UTC;
ICONST_1
ANEWARRAY com/microsoft/sqlserver/jdbc/UTC
DUP
ICONST_0
GETSTATIC com/microsoft/sqlserver/jdbc/UTC.INSTANCE : Lcom/microsoft/sqlserver/jdbc/UTC;
AASTORE
PUTSTATIC com/microsoft/sqlserver/jdbc/UTC.ENUM$VALUES : [Lcom/microsoft/sqlserver/jdbc/UTC;
L1
LINENUMBER 524 L1
NEW java/util/SimpleTimeZone
DUP
ICONST_0
LDC "UTC"
INVOKESPECIAL java/util/SimpleTimeZone.<init> (ILjava/lang/String;)V
PUTSTATIC com/microsoft/sqlserver/jdbc/UTC.timeZone : Ljava/util/TimeZone;
RETURN
MAXSTACK = 4
MAXLOCALS = 0
```
…into SqlVariant # Conflicts: # src/test/java/com/microsoft/sqlserver/jdbc/parametermetadata/ParameterMetaDataTest.java # src/test/java/com/microsoft/sqlserver/testframework/Utils.java
…enum UTC should be a class
| private SqlVariant_ProbBytes(int intValue) { | ||
| this.intValue = intValue; | ||
| } | ||
|
|
There was a problem hiding this comment.
let's move private SqlVariant_ProbBytes(int intValue) up since it's the constructor for the enum. Also change the name of intValue() to getIntValue() ?
| this.intValue = intValue; | ||
| } | ||
|
|
||
| static SqlVariant_ProbBytes valueOf(int intValue) throws IllegalArgumentException { |
There was a problem hiding this comment.
we can remove throws IllegalArgumentException
| private int baseType; | ||
| private int cbPropsActual; | ||
| private int properties; | ||
| private int value; |
There was a problem hiding this comment.
cbPropsActual, properties, value those 3 variables are not used
| /** | ||
| * Constructor for sqlVariant | ||
| */ | ||
| SqlVariant(int baseType) { |
There was a problem hiding this comment.
let's move constructor up as the first method in the class
| * | ||
| * @return | ||
| */ | ||
| int getScale() { |
There was a problem hiding this comment.
let's put getters next to their setters
| TVP tvpValue) throws SQLServerException; | ||
|
|
||
| abstract void execute(DTV dtv, | ||
| SqlVariant SqlVariantValue) throws SQLServerException; |
There was a problem hiding this comment.
SqlVariantValue should be sqlVariantValue
| private Integer scale; | ||
| private boolean forceEncrypt; | ||
|
|
||
| private int variantInternal; |
There was a problem hiding this comment.
this variable is not used
| /** | ||
| * Sets the internal datatype of variant type | ||
| * @param type sql_variant internal type | ||
| */ |
| public static final int GUID = -145; | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
forget to add description?
| con.close(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
let's also close all the closeable objects, e.g. SQLServerResultSet SQLServerBulkCopy
|
REALLY GREAT WORK @v-afrafi !!! I am very amazed to see this complicated feature is implemented now. A complicated feature that involves so many data types and features (e.g. BulkCopy and TVP). All the code review comments are just coding/naming convention, formatting issues, and a few questions/doubts in my mind. Also great to know it does not cause regression in our test lab. |
|
@sehrope, if you do get a chance to code review, please let us know. We look forward to your feedback. |
|
Hello @sehrope, if you don't have time to view it for now, please feel free to create new issue or leave comments :) we have to merge this PR very soon. Thank you 👍 |
…lVariant # Conflicts: # src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java
No description provided.