Custom query fix #1428
Custom query fix #1428
Conversation
Custom query change as per review
| private static NeptuneConnection createConnection(java.util.Map<String, String> configOptions) | ||
| { | ||
| GraphType graphType = GraphType.PROPERTYGRAPH; | ||
| if (System.getenv("neptune_graphtype") != null) { |
There was a problem hiding this comment.
use configOptions.getEnv here. We dump System.getEnv to configOptions when the RecordHandler is instantiated.
| switch(graphType){ | ||
| case PROPERTYGRAPH: | ||
| return new NeptuneGremlinConnection(configOptions.get("neptune_endpoint"), | ||
| configOptions.get("neptune_port"), Boolean.parseBoolean(configOptions.get("iam_enabled")), |
There was a problem hiding this comment.
lets make static variables for these properties.
| break; | ||
|
|
||
| case VIEW: | ||
| String query = recordsRequest.getSchema().getCustomMetadata().get("query"); |
There was a problem hiding this comment.
add static variable here pls. Maybe its worth creating a class for constants.
| this.connectionString = "https://" + neptuneEndpoint + ":" + neptunePort; | ||
| } | ||
|
|
||
| public void connect() throws NeptuneSigV4SignerException |
There was a problem hiding this comment.
why does this need a separate connect() class? why not include this as a part of the constructor?
There was a problem hiding this comment.
connect now from constructor
| @@ -29,7 +29,8 @@ | |||
| </Appenders> | |||
| <Loggers> | |||
| <Logger name="com.amazonaws.athena.connector.lambda" level="${env:ATHENA_FEDERATION_SDK_LOG_LEVEL:-warn}" /> | |||
| <Logger name="com.amazonaws.athena.connectors.neptune" level="${env:ATHENA_FEDERATION_SDK_LOG_LEVEL:-warn}" /> | |||
| <Logger name="com.amazonaws.athena.connectors.neptune" level="warn" /> | |||
There was a problem hiding this comment.
please revert the changes and keep as: level="${env:ATHENA_FEDERATION_SDK_LOG_LEVEL:-warn}
This is used for customer debugging so that log levels can be configured via lambda env variable
| String pfx = k.substring(PREFIX_LEN); | ||
| String val = recordsRequest.getSchema().getCustomMetadata().get(k); | ||
| prefixMap.put(pfx, val); | ||
| prefixBlock += "PREFIX " + pfx + ": <" + val + ">\n"; |
There was a problem hiding this comment.
Please use a stringbuilder for readability or string.format.
There was a problem hiding this comment.
now using string builder
| { | ||
| ArrowType arrowType = field.getType(); | ||
| Types.MinorType minorType = Types.getMinorTypeForArrowType(arrowType); | ||
| Boolean enableCaseinsensitivematch = (System.getenv("enable_caseinsensitivematch") == null) ? true : Boolean.parseBoolean(System.getenv("enable_caseinsensitivematch")); |
There was a problem hiding this comment.
you'll need to use configOptions here. Using System.getEnv within the connectors isn't recommended.
| } | ||
|
|
||
| // 2. Build the SPARQL query | ||
| String queryMode = recordsRequest.getSchema().getCustomMetadata().get("querymode"); |
There was a problem hiding this comment.
pls use constants for all of these. Its hard to determine where they're coming from without prior context.
|
Made code changes to address review comments |
|
build failure |
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
looks like some merge headers got added
There was a problem hiding this comment.
Removed merge headers
Issue #, if available:
#1398
Description of changes:
Add RDF support to Neptune connector. This builds on recent changes for property graph views in Neptune connector.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.