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.

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

[feature] extend range index conditions #1252

Merged
merged 3 commits into from Mar 4, 2017

Conversation

Projects
None yet
5 participants
@olvidalo
Contributor

olvidalo commented Feb 8, 2017

This adds a few extensions to the attribute range index condition introduced in #1233:

  • ability to specify an operator for the condition, implemented operators areeq, ne, lt, gt, le, ge, starts-with, ends-with, contains, matches. Can be specified using an operator attribute on the condition element. If the attribute is missing, eq is assumed as default.

  • case insensitive comparison by specifing case="no" on the condition element

  • numeric comparison by specifing numeric="yes". When enabled, 01.0 will equal 1 and 2 will be less than 110, for example. By default, string matching is assumed. The rewriter will respect the type of the value (string, numeric) when matching a condition to a predicate.

  • rewriting improvements:

    • the order of attribute name and value can be reversed in the predicate, respecting semantics of the operator ([@a = "x"] and ["x" = @a] will both match a condition on a, for example)
    • variables and function calls in predicates will be resolved to a value
  • some refactoring:

    • RangeIndexConfigCondition is now an abstract base class with RangeIndexConfigAttributeCondition being a subclass. This should make it easier to implement other types of conditions in the future
    • moved most of the rewriting code from OptimizeFieldPragma to the condition itself as it will differ for different kinds of conditions

I'll be happy to add some documentation on this to the "New Range Index" page later on...

@joewiz

This comment has been minimized.

Member

joewiz commented Feb 12, 2017

Looks great! Thanks again for contributing these really significant enhancements!

@adamretter adamretter self-requested a review Feb 21, 2017

} else if (expr instanceof VariableReference || expr instanceof Function) {
try {
final Sequence result = expr.eval(null);

This comment has been minimized.

@olvidalo

olvidalo Feb 22, 2017

Contributor

Now that this is getting some attention... :)

I'm wondering if this could be a problem if someone would, for whatever reason, put an updating function in the predicate or something else with side effects. Maybe the expression could be evaluated in some kind of read only copy of the xquery context? Or wouldn't that be necessary?

I also thought about possible security risks of evaluating an expression during rewriting, but as it is evaluated with the same rights as the script itself, that shouldn't be a problem per se, right?

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

You don't need to worry about the security, the same user as executed the query will be used.

@@ -162,9 +163,9 @@ public boolean matchConditions(Node node) {
return true;
}
public boolean findCondition(QName lhe, String rhe, RangeIndex.Operator operator) {
public boolean findCondition(Predicate predicate) {

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

In future please consider using final on all variables that are not reassigned.

private static final Map<String, Operator> LOOKUP_MAP;
static {
LOOKUP_MAP = new HashMap<String, Operator>();

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

Could you use Java 7 Diamond Syntax here?

@@ -0,0 +1,405 @@
/*
* eXist Open Source Native XML Database
* Copyright (C) 2013 The eXist Project

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

Should be 2017

}
this.attributeName = elem.getAttribute("attribute");
if (this.attributeName == null || this.attributeName.length() == 0) {

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

Use isEmpty() and not length() == 0

try {
this.pattern = Pattern.compile(this.value, flags);
} catch (PatternSyntaxException e) {
RangeIndex.LOG.error(e);

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

Probably better to have RangeIndex.LOG as private and declare your own logger for this class.

@adamretter adamretter merged commit 9c2357d into eXist-db:develop Mar 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adamretter

This comment has been minimized.

Member

adamretter commented Mar 4, 2017

@olvidalo Thanks for your contributions. I had a couple small comments for future reference, but otherwise this looks nice and clean. Would love to see some documentation added to https://github.com/eXist-db/documentation/ ;-)

@olvidalo olvidalo referenced this pull request Mar 14, 2017

Merged

Range index conditions #100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment