Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update order of vertexes, and add closing vertex for Oracle. #142

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Update order of vertexes, and add closing vertex for Oracle. #142

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
15d29b6
Update order of vertexes, and add closing vertex for Oracle.
at88mph Feb 24, 2023
2a9145b
Fix for namespace change.
at88mph Feb 28, 2023
5a5d8ef
Test fix in cadc-adql.
at88mph Feb 28, 2023
6d7f256
Reversed namespace change.
at88mph Mar 4, 2023
03f57cb
Merge branch 'master' into polygon-vertex-order-fix
pdowler Mar 6, 2023
63cffbe
Merge branch 'master' of https://github.com/opencadc/tap into polygon…
at88mph Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions cadc-adql/src/test/java/ca/nrc/cadc/tap/permissions/TapSchemaReadAccessConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@

import javax.security.auth.Subject;

import ca.nrc.cadc.auth.NotAuthenticatedException;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
import org.junit.Assert;
Expand Down Expand Up @@ -512,15 +513,20 @@ public Object toOwner(Subject subject) {
}

@Override
public int getOwnerType() {
return 0;
public Subject validate(Subject subject) throws NotAuthenticatedException {
return subject;
}

@Override
public String toOwnerString(Subject subject) {
public Subject augment(Subject subject) {
return subject;
}

@Override
public String toDisplayString(Subject subject) {
return null;
}

}

static class TestGMSClient implements GroupClient {
Expand Down
2 changes: 1 addition & 1 deletion cadc-tap-server-oracle/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ repositories {
apply from: '../opencadc.gradle'
sourceCompatibility = 1.8
group = 'org.opencadc'
version = '1.2.11'
version = '1.2.12'

description = 'OpenCADC TAP-1.1 tap server plugin (Oracle)'
def git_url = 'https://github.com/opencadc/tap'
Expand Down
93 changes: 67 additions & 26 deletions ...tap-server-oracle/src/main/java/ca/nrc/cadc/tap/parser/region/function/OraclePolygon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,23 +71,24 @@

import ca.nrc.cadc.dali.Point;
import ca.nrc.cadc.dali.Polygon;
import ca.nrc.cadc.tap.parser.RegionFinder;

import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.stream.Collectors;

import net.sf.jsqlparser.expression.DoubleValue;
import net.sf.jsqlparser.expression.Expression;
import net.sf.jsqlparser.expression.LongValue;
import net.sf.jsqlparser.expression.StringValue;
import net.sf.jsqlparser.expression.operators.relational.ExpressionList;


/**
* Represents an Oracle Polygon object.
*/
public class OraclePolygon extends OracleGeometricFunction {

private static final int[] ORACLE_ELEMENT_INFO_VALUES = new int[] {
1, 2003, 1
};
Expand All @@ -101,55 +102,95 @@ public class OraclePolygon extends OracleGeometricFunction {
}
}

private final List<Expression> vertices = new ArrayList<>();
private final List<Expression> vertexExpressions = new ArrayList<>();


private OraclePolygon() {
super(ORACLE_ELEMENT_INFO);
}

public OraclePolygon(final List<Expression> verticeExpressions) {
public OraclePolygon(final List<Expression> vertexExpressions) {
this();
if (verticeExpressions != null) {
this.vertices.addAll(verticeExpressions);
if (vertexExpressions != null) {
// Skip the first value as it's the coord sys (e.g. ICRS).
this.vertexExpressions.addAll(vertexExpressions.subList(1, vertexExpressions.size()));
}
ensureCounterClockwise();
processOrdinateParameters();
}

public OraclePolygon(final Polygon polygon) {
this();
vertices.add(new StringValue(RegionFinder.ICRS));
for (final Point p : polygon.getVertices()) {
vertices.add(new DoubleValue(Double.toString(p.getLongitude())));
vertices.add(new DoubleValue(Double.toString(p.getLatitude())));
vertexExpressions.add(new DoubleValue(Double.toString(p.getLongitude())));
vertexExpressions.add(new DoubleValue(Double.toString(p.getLatitude())));
}
ensureCounterClockwise();
processOrdinateParameters();
}

private void ensureCounterClockwise() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the algorithm here is for a polygon on a flat plane, so is approximate. I don't know what "edgeSum" is but this looks less complex than the code in caom2 which does this by computing the area via triangulation.

Is this calculation going to work in edge cases:

  1. near or includes the pole
  2. overlaps the meridian (longitude=0) so some vertices have longitude near 360 and others near 0
  3. is is larger than half the sphere

It's probably OK to not support #3 (for now), but it should be detected and rejected. 1 and 2 need to be taken into account somehow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. The sum of the edges indicates a clockwise direction if it's a positive number, and counter-clockwise if it's negative, and assumes a flat plane. I hadn't realized it needed to be more complex than that, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the GrahamScan in CAOM-2 a place to start learning about how to handle those three cases? Is this the "winding" method?

double edgeSum = 0.0D;
final int length = this.vertexExpressions.size();
for (int i = 0; i < length; i = i + 2) {
final Expression ra1 = this.vertexExpressions.get(i);
final Expression dec1 = this.vertexExpressions.get(i + 1);

final Expression ra2;
final Expression dec2;
if (i == length - 2) {
// Back to the beginning if we're on the last vertex.
ra2 = this.vertexExpressions.get(0);
dec2 = this.vertexExpressions.get(1);
} else {
// Back to the beginning if we're on the last vertex.
ra2 = this.vertexExpressions.get(i + 2);
dec2 = this.vertexExpressions.get(i + 3);
}
edgeSum += (Double.parseDouble(ra2.toString()) - Double.parseDouble(ra1.toString())) *
(Double.parseDouble(dec2.toString()) + Double.parseDouble(dec1.toString()));
}

if (edgeSum > 0.0D) {
final List<Expression> reversedVertices = new ArrayList<>();
// Clockwise, so reverse the vertices, but maintain the pairs of points.
for (int i = length - 2; i >= 0; i -= 2) {
reversedVertices.add(this.vertexExpressions.get(i));
reversedVertices.add(this.vertexExpressions.get(i + 1));
}

this.vertexExpressions.clear();
this.vertexExpressions.addAll(reversedVertices);
}
}

/**
* Map this shape's values to ORACLE ORDINATE function parameters.
*
* @param parameterList The ExpressionList to add parameters to.
*/
@Override
@SuppressWarnings("unchecked")
void mapValues(final ExpressionList parameterList) {
// Start at 1 since the first item will be the coordinate system.
for (int i = 1; i < this.vertices.size(); i = i + 2) {
final Expression ra = this.vertices.get(i);
final Expression dec = this.vertices.get(i + 1);
addNumericExpression(ra, parameterList);
addNumericExpression(dec, parameterList);
final int vertexCount = this.vertexExpressions.size();
for (int i = 0; i < vertexCount; i = i + 2) {
final Expression ra = this.vertexExpressions.get(i);
final Expression dec = this.vertexExpressions.get(i + 1);
parameterList.getExpressions().add(ra);
parameterList.getExpressions().add(dec);
}
}

@SuppressWarnings("unchecked")
void addNumericExpression(final Expression expression, final ExpressionList parameterList) {
if (!(expression instanceof DoubleValue) && !(expression instanceof LongValue)) {
throw new UnsupportedOperationException(
String.format("Cannot use non-constant coordinates in Polygon. Expected Double or Long but found"
+ " '%s'", expression.toString()));
} else {
parameterList.getExpressions().add(expression);
final Expression firstVertexXExpression = this.vertexExpressions.get(0);
final Expression firstVertexYExpression = this.vertexExpressions.get(1);
final double firstVertexX = Double.parseDouble(firstVertexXExpression.toString());
final double firstVertexY = Double.parseDouble(firstVertexYExpression.toString());
final double lastVertexX = Double.parseDouble(this.vertexExpressions.get(vertexCount - 2).toString());
final double lastVertexY = Double.parseDouble(this.vertexExpressions.get(vertexCount - 1).toString());

// Close the Polygon, if necessary, as per Oracle's requirements.
if (firstVertexX != lastVertexX || firstVertexY != lastVertexY) {
parameterList.getExpressions().add(firstVertexXExpression);
parameterList.getExpressions().add(firstVertexYExpression);
}
}

Expand All @@ -164,6 +205,6 @@ public static boolean structMatches(final BigDecimal[] structTypeArray) {
final List<Integer> currValues = Arrays.stream(ORACLE_ELEMENT_INFO_VALUES).boxed().collect(Collectors.toList());
final List<Integer> structValues = Arrays.stream(structTypeArray).map(BigDecimal::intValue).collect(
Collectors.toList());
return structValues.containsAll(currValues);
return new HashSet<>(structValues).containsAll(currValues);
}
}
15 changes: 14 additions & 1 deletion ...server-oracle/src/test/java/ca/nrc/cadc/tap/parser/region/function/OraclePolygonTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,18 @@ public void convertParameters() {
final Function ordinateArrayFunction = new Function();
final ExpressionList ordinateArrayFunctionParams = new ExpressionList(new ArrayList<>());
ordinateArrayFunction.setName(OraclePolygon.ORDINATE_ARRAY_FUNCTION_NAME);
ordinateArrayFunctionParams.getExpressions().addAll(expressionList.subList(1, expressionList.size()));

final List<Expression> expectedExpressionList = new ArrayList<>();
expectedExpressionList.add(new DoubleValue("288.0"));
expectedExpressionList.add(new DoubleValue("28.0"));
expectedExpressionList.add(new DoubleValue("288.0"));
expectedExpressionList.add(new DoubleValue("388.0"));
expectedExpressionList.add(new DoubleValue("88.0"));
expectedExpressionList.add(new DoubleValue("188.0"));
expectedExpressionList.add(new DoubleValue("288.0"));
expectedExpressionList.add(new DoubleValue("28.0"));

ordinateArrayFunctionParams.getExpressions().addAll(expectedExpressionList);
ordinateArrayFunction.setParameters(ordinateArrayFunctionParams);

expectedExpressions.add(new LongValue("2003"));
Expand Down Expand Up @@ -140,6 +151,8 @@ public void convertParametersFromPolygon() {
expectedVertices.add(new DoubleValue("77.9"));
expectedVertices.add(new DoubleValue("20.0"));
expectedVertices.add(new DoubleValue("-0.8"));
expectedVertices.add(new DoubleValue("44.0"));
expectedVertices.add(new DoubleValue("89.8"));

ordinateArrayFunction.setName(OraclePolygon.ORDINATE_ARRAY_FUNCTION_NAME);
ordinageArrayFunctionParams.getExpressions().addAll(expectedVertices);
Expand Down
4 changes: 2 additions & 2 deletions ...ap-server-oracle/src/test/java/ca/nrc/cadc/tap/writer/format/OraclePolygonFormatTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public void getPolygon() {
final Polygon polygon = testSubject.getPolygon(OraclePolygonFormat.POLYGON_FUNCTION
+ "(0.9, 4.6, 9.0, 10.2, 3.3, 8.7, 12.9, 45.6)");
final List<Point> vertices = polygon.getVertices();
assertEquals("Wrong vertice count.", 4, vertices.size());
assertEquals("Wrong vertex count.", 4, vertices.size());
}

@Test
Expand All @@ -119,7 +119,7 @@ public void format() {
polygon.getVertices().add(new Point(88.4D, -19.2D));
polygon.getVertices().add(new Point(8.6D, 45.3D));

assertEquals("Wrong output.", "Polygon 88.0 90.0 88.4 -19.2 8.6 45.3", testSubject.format(polygon));
assertEquals("Wrong output.", "Polygon 8.6 45.3 88.4 -19.2 88.0 90.0", testSubject.format(polygon));

assertEquals("Wrong output from null input.", "", testSubject.format(null));
}
Expand Down
5 changes: 2 additions & 3 deletions cadc-tap/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ plugins {
id "java"
id "maven"
id 'maven-publish'
id 'com.jfrog.bintray' version '1.8.4'
id 'checkstyle'
}

Expand All @@ -16,7 +15,7 @@ apply from: '../opencadc.gradle'
sourceCompatibility = 1.8

group = 'org.opencadc'
version = '1.1.15'
version = '1.1.16'

description = 'OpenCADC TAP-1.1 tap client library'
def git_url = 'https://github.com/opencadc/tap'
Expand All @@ -25,7 +24,7 @@ dependencies {
compile 'org.apache.commons:commons-csv:[1.6,1.7)'

compile 'org.opencadc:cadc-util:[1.6.3,2.0)'
compile 'org.opencadc:cadc-registry:[1.5,2.0)'
compile 'org.opencadc:cadc-registry:[1.7.1,2.0)'
compile 'org.opencadc:cadc-dali:[1.2,2.0)'
compile 'org.opencadc:cadc-vosi:[1.0,2.0)'
compile 'org.opencadc:cadc-gms:[1.0,)'
Expand Down
4 changes: 2 additions & 2 deletions cadc-tap/src/main/java/ca/nrc/cadc/vosi/TableReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ public class TableReader extends TableSetParser
{
private static final Logger log = Logger.getLogger(TableReader.class);

private static final String TAP_TYPE = "vod:TAPType";
private static final String VOT_TYPE = "vod:VOTableType";
private static final String TAP_TYPE = "vs:TAPType";
at88mph marked this conversation as resolved.
Show resolved Hide resolved
private static final String VOT_TYPE = "vs:VOTableType";

public TableReader() { this(true); }

Expand Down