Skip to content

Commit

Permalink
Merge pull request #264 from alfasoftware/fix/deprecate-oraclecustomh…
Browse files Browse the repository at this point in the history
…int-and-postgresqlcustomhint

Fix/deprecate oraclecustomhint and postgresqlcustomhint
  • Loading branch information
gilleain authored Jun 16, 2023
2 parents 11e8831 + f35cee9 commit 6ffa28b
Show file tree
Hide file tree
Showing 13 changed files with 256 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@
*
* Interface for Custom hints
*
* @deprecated This interface and its implementing classes should be removed in the near future as platform specific classes should be outside of core project
*
* @author Copyright (c) Alfa Financial Software Limited. 2021
*/
@Deprecated
public interface CustomHint extends Hint {

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package org.alfasoftware.morf.sql;

import java.util.Objects;

import org.apache.commons.lang3.StringUtils;

/**
* A generic {@link org.alfasoftware.morf.sql.Hint} class that holds custom hints for a given database type.
* It should be used instead of {@link org.alfasoftware.morf.sql.CustomHint}
*
*/
public class DialectSpecificHint implements Hint {

private final String databaseType;

private final String hintContents;

/**
*
* @param databaseType a database type identifier. Eg: ORACLE, PGSQL, SQL_SERVER
* @param hintContents the hint contents themselves, without the delimiters. Eg: without /*+ and *"/ * for Oracle hints
*/
public DialectSpecificHint(String databaseType, String hintContents) {
super();

if (StringUtils.isBlank(databaseType)) {
throw new IllegalArgumentException("databaseType cannot be blank");
}

if (StringUtils.isBlank(hintContents)) {
throw new IllegalArgumentException("hintContents cannot be blank");
}

this.databaseType = databaseType;
this.hintContents = hintContents;
}


public String getDatabaseType() {
return databaseType;
}


public String getHintContents() {
return hintContents;
}


/**
* Tests whether the supplied databaseType string is equal to the databaseType that is held by this class.
* The test is performed using {@link java.lang.String#equals(Object)}.
*
* @param databaseType a database type identifier. Eg: ORACLE, PGSQL, SQL_SERVER
* @return true if the databaseType parameter is equal to this databaseType, false otherwise
*/
public boolean isSameDatabaseType(String databaseType) {
return this.databaseType.equals(databaseType);
}

@Override
public int hashCode() {
return Objects.hash(databaseType, hintContents);
}


@Override
public boolean equals(Object obj) {
if (this == obj) return true;
if (obj == null) return false;
if (getClass() != obj.getClass()) return false;
DialectSpecificHint other = (DialectSpecificHint) obj;
return Objects.equals(databaseType, other.databaseType) && Objects.equals(hintContents, other.hintContents);
}


@Override
public String toString() {
return "DialectSpecificHint [databaseType=" + databaseType + ", hintContents=" + hintContents + "]";
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@

/**
* Represents a custom hint for a query
*
* @deprecated See {@link org.alfasoftware.morf.sql.CustomHint}
*/
@Deprecated
public final class OracleCustomHint implements CustomHint {

private final String customHint;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

/**
* Represents a custom hint for a query
*
* @deprecated See {@link org.alfasoftware.morf.sql.CustomHint}
*/
@Deprecated
public final class PostgreSQLCustomHint implements CustomHint {

private final String customHint;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -477,14 +477,30 @@ public SelectStatement allowParallelDml() {
* @param customHint representation of a custom hint
*
* @return this, for method chaining.
* @deprecated Use {@link #withDialectSpecificHint(String, String)} instead. See why this is deprecated at {@link org.alfasoftware.morf.sql.CustomHint}
*/
@Deprecated
public SelectStatement withCustomHint(CustomHint customHint) {
return copyOnWriteOrMutate(
(SelectStatementBuilder b) -> b.withCustomHint(customHint),
() -> this.hints.add(customHint)
);
}

/**
* Supplies a specified custom hint to the database for a query.
*
* @param databaseType a database type identifier. Eg: ORACLE, PGSQL, SQL_SERVER
* @param hintContents the hint contents themselves, without the delimiters. Eg: without /*+ and *"/ * for Oracle hints
* @return this, for method chaining.
*/
public SelectStatement withDialectSpecificHint(String databaseType, String hintContents) {
return copyOnWriteOrMutate(
(SelectStatementBuilder b) -> b.withDialectSpecificHint(databaseType, hintContents),
() -> this.hints.add(new DialectSpecificHint(databaseType, hintContents))
);
}


/**
* If supported by the dialect, hints to the database that joins should be applied in the order
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,13 +314,29 @@ public SelectStatementBuilder allowParallelDml() {
*
* @param customHint representation of a custom hint
* @return this, for method chaining.
* @deprecated See {@link org.alfasoftware.morf.sql.CustomHint}
*/
@Deprecated
public org.alfasoftware.morf.sql.SelectStatementBuilder withCustomHint(CustomHint customHint) {
this.hints.add(customHint);
return this;
}


/**
* Supplies a specified custom hint and database type to the database for a query.
*
* @param databaseType a database type identifier. Eg: ORACLE, PGSQL, SQL_SERVER
* @param hintContents the hint contents themselves, without the delimiters. Eg: without /*+ and *"/ * for Oracle hints
* @return this, for method chaining.
*/
public org.alfasoftware.morf.sql.SelectStatementBuilder withDialectSpecificHint(String databaseType, String hintContents) {
DialectSpecificHint dialectSpecificHint = new DialectSpecificHint(databaseType, hintContents);
this.hints.add(dialectSpecificHint);
return this;
}


/**
* If supported by the dialect, hints to the database that joins should be applied in the order
* they are written in the SQL statement.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package org.alfasoftware.morf.sql;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import org.junit.Test;

public class TestDialectSpecificHint {

/**
* We should not be able to create a DialectSpecificHint object with blank parameters
*/
@Test(expected = IllegalArgumentException.class)
public void testConstructorWithFirstParameterEmptyThrowsException() {
new DialectSpecificHint("", "not_empty");
fail("Did not catch IllegalArgumentException");
}


/**
* We should not be able to create a DialectSpecificHint object with blank parameters
*/
@Test(expected = IllegalArgumentException.class)
public void testConstructorWithSecondParameterEmptyThrowsException() {
new DialectSpecificHint("not_empty", "");
fail("Did not catch IllegalArgumentException");
}

@Test
public void testIsSameDatabaseType() {
DialectSpecificHint dialectSpecificHint = new DialectSpecificHint("SOME_DATABASE_TYPE", "SOME_HINT");
assertTrue(dialectSpecificHint.isSameDatabaseType("SOME_DATABASE_TYPE"));
assertFalse(dialectSpecificHint.isSameDatabaseType("DIFFERENT_DATABASE_TYPE"));
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -1081,13 +1081,15 @@ public void testMutableBuilder() {
.useImplicitJoinOrder()
.useIndex(table1, "INDEX_1")
.useIndex(table2, "INDEX_2")
.allowParallelDml();
.allowParallelDml()
.withDialectSpecificHint("ORACLE", "index(customer cust_primary_key_idx)");
assertThat(select.getHints(), contains(
new OptimiseForRowCount(1),
new UseImplicitJoinOrder(),
new UseIndex(table1, "INDEX_1"),
new UseIndex(table2, "INDEX_2"),
AllowParallelDmlHint.INSTANCE
AllowParallelDmlHint.INSTANCE,
new DialectSpecificHint("ORACLE", "index(customer cust_primary_key_idx)")
));

});
Expand Down Expand Up @@ -1410,28 +1412,32 @@ public void testHintsImmutable() {
.useIndex(table1, "INDEX_1")
.useIndex(table2, "INDEX_2")
.allowParallelDml()
.withDialectSpecificHint("ORACLE", "index(customer cust_primary_key_idx)")
.build()
.getHints(), contains(
new OptimiseForRowCount(1),
new UseImplicitJoinOrder(),
new UseIndex(table1, "INDEX_1"),
new UseIndex(table2, "INDEX_2"),
AllowParallelDmlHint.INSTANCE
AllowParallelDmlHint.INSTANCE,
new DialectSpecificHint("ORACLE", "index(customer cust_primary_key_idx)")
));

// Implicit copy-on-write
SelectStatement updated = select.optimiseForRowCount(1)
.useImplicitJoinOrder()
.useIndex(table1, "INDEX_1")
.useIndex(table2, "INDEX_2")
.allowParallelDml();
.allowParallelDml()
.withDialectSpecificHint("ORACLE", "index(customer cust_primary_key_idx)");

assertThat(updated.getHints(), contains(
new OptimiseForRowCount(1),
new UseImplicitJoinOrder(),
new UseIndex(table1, "INDEX_1"),
new UseIndex(table2, "INDEX_2"),
AllowParallelDmlHint.INSTANCE
AllowParallelDmlHint.INSTANCE,
new DialectSpecificHint("ORACLE", "index(customer cust_primary_key_idx)")
));
assertThat(select.getHints(), empty());
assertUnsupported(() -> updated.getHints().add(new OptimiseForRowCount(3)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.alfasoftware.morf.metadata.SchemaUtils;
import org.alfasoftware.morf.metadata.Table;
import org.alfasoftware.morf.metadata.View;
import org.alfasoftware.morf.sql.DialectSpecificHint;
import org.alfasoftware.morf.sql.DirectPathQueryHint;
import org.alfasoftware.morf.sql.ExceptSetOperator;
import org.alfasoftware.morf.sql.Hint;
Expand Down Expand Up @@ -1318,7 +1319,7 @@ protected String selectStatementPreFieldDirectives(SelectStatement selectStateme
.append(((OptimiseForRowCount)hint).getRowCount())
.append(")");
}
if (hint instanceof UseIndex) {
else if (hint instanceof UseIndex) {
UseIndex useIndex = (UseIndex)hint;
builder.append(" INDEX(")
// No schema name - see http://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements006.htm#BABIEJEB
Expand All @@ -1327,21 +1328,25 @@ protected String selectStatementPreFieldDirectives(SelectStatement selectStateme
.append(useIndex.getIndexName())
.append(")");
}
if (hint instanceof UseImplicitJoinOrder) {
else if (hint instanceof UseImplicitJoinOrder) {
builder.append(" ORDERED");
}
if (hint instanceof ParallelQueryHint) {
else if (hint instanceof ParallelQueryHint) {
builder.append(" PARALLEL");
ParallelQueryHint parallelQueryHint = (ParallelQueryHint) hint;
builder.append(parallelQueryHint.getDegreeOfParallelism().map(d -> "(" + d + ")").orElse(""));
}
if (hint instanceof AllowParallelDmlHint) {
else if (hint instanceof AllowParallelDmlHint) {
builder.append(" ENABLE_PARALLEL_DML");
}
if (hint instanceof OracleCustomHint) {
else if (hint instanceof OracleCustomHint) {
builder.append(" ")
.append(((OracleCustomHint)hint).getCustomHint());
}
else if ( hint instanceof DialectSpecificHint && ((DialectSpecificHint)hint).isSameDatabaseType(Oracle.IDENTIFIER) ) {
builder.append(" ")
.append(((DialectSpecificHint)hint).getHintContents());
}
}

if (builder.length() == 0) {
Expand Down Expand Up @@ -1393,7 +1398,7 @@ public int legacyFetchSizeForBulkSelects() {
return 200;
}


/**
* We do use NVARCHAR for strings on Oracle.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1508,6 +1508,7 @@ public List<String> expectedAddTableFromStatements() {
}


@Override
public List<String> expectedReplaceTableFromStatements() {
return ImmutableList.of(
"CREATE TABLE TESTSCHEMA.tmp_SomeTable (someField NOT NULL, otherField NOT NULL, thirdField NOT NULL, CONSTRAINT tmp_SomeTable_PK PRIMARY KEY (someField) USING INDEX (CREATE UNIQUE INDEX TESTSCHEMA.tmp_SomeTable_PK ON TESTSCHEMA.tmp_SomeTable (someField))) PARALLEL NOLOGGING AS SELECT CAST(someField AS NVARCHAR2(3)) AS someField, CAST(otherField AS DECIMAL(3,0)) AS otherField, CAST(thirdField AS DECIMAL(5,0)) AS thirdField FROM TESTSCHEMA.OtherTable",
Expand All @@ -1528,6 +1529,7 @@ public List<String> expectedReplaceTableFromStatements() {
}


@Override
public List<String> expectedReplaceTableWithAutonumber() {
return ImmutableList.of(
"CREATE TABLE TESTSCHEMA.tmp_SomeTable (someField NOT NULL, otherField NOT NULL, thirdField NOT NULL, CONSTRAINT tmp_SomeTable_PK PRIMARY KEY (someField) USING INDEX (CREATE UNIQUE INDEX TESTSCHEMA.tmp_SomeTable_PK ON TESTSCHEMA.tmp_SomeTable (someField))) PARALLEL NOLOGGING AS SELECT CAST(someField AS NVARCHAR2(3)) AS someField, CAST(otherField AS DECIMAL(3,0)) AS otherField, CAST(thirdField AS DECIMAL(5,0)) AS thirdField FROM TESTSCHEMA.OtherTable",
Expand Down Expand Up @@ -1688,6 +1690,23 @@ protected CustomHint provideCustomHint() {
}


/**
* @see org.alfasoftware.morf.jdbc.AbstractSqlDialectTest#expectedHints8a()
*/
@Override
protected String expectedHints8a() {
return "SELECT /*+ index(customer cust_primary_key_idx) */ * FROM SCHEMA2.Foo";
}


/**
* @see org.alfasoftware.morf.jdbc.AbstractSqlDialectTest#provideDatabaseType()
*/
@Override
protected String provideDatabaseType() {
return Oracle.IDENTIFIER;
}


@Override
protected boolean expectedUsesNVARCHARforStrings() {
Expand Down
Loading

0 comments on commit 6ffa28b

Please sign in to comment.