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 default precision step, modulo tests #5908

Closed
wants to merge 4 commits into from
Closed

Update default precision step, modulo tests #5908

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0bee444
update default precision step, modulo tests
Apr 22, 2014
67fc81c
cleanups from pr comments
Apr 22, 2014
80efba6
fix documentation to refer to the new defaults, also fix description …
Apr 22, 2014
31a9c09
add tests for precision steps, fix mapper to actually set this in the…
Apr 23, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion core-signatures.txt
View file
Open in desktop
Expand Up @@ -34,6 +34,16 @@ org.apache.lucene.index.IndexWriter#forceMergeDeletes(boolean) @ use Merges#forc
@defaultMessage QueryWrapperFilter is cachable by default - use Queries#wrap instead
org.apache.lucene.search.QueryWrapperFilter#<init>(org.apache.lucene.search.Query)

@defaultMessage Pass the precision step from the mappings explicitly instead
org.apache.lucene.search.NumericRangeQuery#newDoubleRange(java.lang.String,java.lang.Double,java.lang.Double,boolean,boolean)
Copy link
Contributor

Choose a reason for hiding this comment

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

man those seem to be trappy - maybe we should deprecate them in Lucene as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see https://issues.apache.org/jira/browse/LUCENE-5605

org.apache.lucene.search.NumericRangeQuery#newFloatRange(java.lang.String,java.lang.Float,java.lang.Float,boolean,boolean)
org.apache.lucene.search.NumericRangeQuery#newIntRange(java.lang.String,java.lang.Integer,java.lang.Integer,boolean,boolean)
org.apache.lucene.search.NumericRangeQuery#newLongRange(java.lang.String,java.lang.Long,java.lang.Long,boolean,boolean)
org.apache.lucene.search.NumericRangeFilter#newDoubleRange(java.lang.String,java.lang.Double,java.lang.Double,boolean,boolean)
org.apache.lucene.search.NumericRangeFilter#newFloatRange(java.lang.String,java.lang.Float,java.lang.Float,boolean,boolean)
org.apache.lucene.search.NumericRangeFilter#newIntRange(java.lang.String,java.lang.Integer,java.lang.Integer,boolean,boolean)
org.apache.lucene.search.NumericRangeFilter#newLongRange(java.lang.String,java.lang.Long,java.lang.Long,boolean,boolean)

@defaultMessage Only use wait / notify when really needed try to use concurrency primitives, latches or callbacks instead.
java.lang.Object#wait()
java.lang.Object#wait(long)
Expand All @@ -46,4 +56,4 @@ java.lang.Math#abs(int)
java.lang.Math#abs(long)

@defaultMessage Use Long.compare instead we are on Java7
com.google.common.primitives.Longs#compare(long,long)
com.google.common.primitives.Longs#compare(long,long)
4 changes: 2 additions & 2 deletions src/main/java/org/elasticsearch/index/analysis/NumericDateAnalyzer.java
View file
Open in desktop
Expand Up @@ -19,7 +19,7 @@

package org.elasticsearch.index.analysis;

import org.apache.lucene.util.NumericUtils;
import org.elasticsearch.index.mapper.core.DateFieldMapper;
import org.joda.time.format.DateTimeFormatter;

import java.io.IOException;
Expand All @@ -35,7 +35,7 @@ public class NumericDateAnalyzer extends NumericAnalyzer<NumericDateTokenizer> {
private final DateTimeFormatter dateTimeFormatter;

public NumericDateAnalyzer(DateTimeFormatter dateTimeFormatter) {
this(NumericUtils.PRECISION_STEP_DEFAULT, dateTimeFormatter);
this(DateFieldMapper.DEFAULT_PRECISION_STEP, dateTimeFormatter);
}

public NumericDateAnalyzer(int precisionStep, DateTimeFormatter dateTimeFormatter) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/elasticsearch/index/analysis/NumericDoubleAnalyzer.java
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@
package org.elasticsearch.index.analysis;

import com.carrotsearch.hppc.IntObjectOpenHashMap;
import org.apache.lucene.util.NumericUtils;
import org.elasticsearch.index.mapper.core.DoubleFieldMapper;

import java.io.IOException;
import java.io.Reader;
Expand Down Expand Up @@ -51,7 +51,7 @@ public static NamedAnalyzer buildNamedAnalyzer(int precisionStep) {
private final int precisionStep;

public NumericDoubleAnalyzer() {
this(NumericUtils.PRECISION_STEP_DEFAULT);
this(DoubleFieldMapper.DEFAULT_PRECISION_STEP);
}

public NumericDoubleAnalyzer(int precisionStep) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/elasticsearch/index/analysis/NumericFloatAnalyzer.java
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@
package org.elasticsearch.index.analysis;

import com.carrotsearch.hppc.IntObjectOpenHashMap;
import org.apache.lucene.util.NumericUtils;
import org.elasticsearch.index.mapper.core.FloatFieldMapper;

import java.io.IOException;
import java.io.Reader;
Expand Down Expand Up @@ -51,7 +51,7 @@ public static NamedAnalyzer buildNamedAnalyzer(int precisionStep) {
private final int precisionStep;

public NumericFloatAnalyzer() {
this(NumericUtils.PRECISION_STEP_DEFAULT);
this(FloatFieldMapper.DEFAULT_PRECISION_STEP);
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems unused anyways I think we should just drop these default ctors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good. I didnt yet look to see how they are used.

}

public NumericFloatAnalyzer(int precisionStep) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/elasticsearch/index/analysis/NumericIntegerAnalyzer.java
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@
package org.elasticsearch.index.analysis;

import com.carrotsearch.hppc.IntObjectOpenHashMap;
import org.apache.lucene.util.NumericUtils;
import org.elasticsearch.index.mapper.core.IntegerFieldMapper;

import java.io.IOException;
import java.io.Reader;
Expand Down Expand Up @@ -51,7 +51,7 @@ public static NamedAnalyzer buildNamedAnalyzer(int precisionStep) {
private final int precisionStep;

public NumericIntegerAnalyzer() {
this(NumericUtils.PRECISION_STEP_DEFAULT);
this(IntegerFieldMapper.DEFAULT_PRECISION_STEP);
}

public NumericIntegerAnalyzer(int precisionStep) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/elasticsearch/index/analysis/NumericLongAnalyzer.java
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@
package org.elasticsearch.index.analysis;

import com.carrotsearch.hppc.IntObjectOpenHashMap;
import org.apache.lucene.util.NumericUtils;
import org.elasticsearch.index.mapper.core.LongFieldMapper;

import java.io.IOException;
import java.io.Reader;
Expand Down Expand Up @@ -51,7 +51,7 @@ public static NamedAnalyzer buildNamedAnalyzer(int precisionStep) {
private final int precisionStep;

public NumericLongAnalyzer() {
this(NumericUtils.PRECISION_STEP_DEFAULT);
this(LongFieldMapper.DEFAULT_PRECISION_STEP);
}

public NumericLongAnalyzer(int precisionStep) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java
View file
Open in desktop
Expand Up @@ -62,6 +62,7 @@
public class ByteFieldMapper extends NumberFieldMapper<Byte> {

public static final String CONTENT_TYPE = "byte";
public static final int DEFAULT_PRECISION_STEP = Integer.MAX_VALUE; // 256 terms max!
Copy link
Contributor

Choose a reason for hiding this comment

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

I think think this constant should be set to Defaults#PRECISION_STEP?

Copy link
Contributor

Choose a reason for hiding this comment

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

aaah nevermind - that is all static.... grrr... It seems like we should add PRECISION_STEP_8BIT, PRECISION_STEP_16BIT etc to NumberFieldMapper#Defaults that woudl be the cleaner solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good. Wasn't sure if we want to do it all based on bitness. In general I have not had time to think about each type. For example, to me 8 is intuitive for an IP-address type, regardless of what the default is for Integer.


public static class Defaults extends NumberFieldMapper.Defaults {
public static final FieldType FIELD_TYPE = new FieldType(NumberFieldMapper.Defaults.FIELD_TYPE);
Expand All @@ -78,7 +79,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, ByteField
protected Byte nullValue = Defaults.NULL_VALUE;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
}

Expand Down Expand Up @@ -346,7 +347,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || precisionStep != Defaults.PRECISION_STEP) {
if (includeDefaults || precisionStep != DEFAULT_PRECISION_STEP) {
builder.field("precision_step", precisionStep);
}
if (includeDefaults || nullValue != null) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java
View file
Open in desktop
Expand Up @@ -70,6 +70,7 @@
public class DateFieldMapper extends NumberFieldMapper<Long> {

public static final String CONTENT_TYPE = "date";
public static final int DEFAULT_PRECISION_STEP = LongFieldMapper.DEFAULT_PRECISION_STEP;

public static class Defaults extends NumberFieldMapper.Defaults {
public static final FormatDateTimeFormatter DATE_TIME_FORMATTER = Joda.forPattern("dateOptionalTime", Locale.ROOT);
Expand Down Expand Up @@ -97,7 +98,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, DateField
private Locale locale;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
// do *NOT* rely on the default locale
locale = Locale.ROOT;
Expand Down Expand Up @@ -523,7 +524,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || precisionStep != Defaults.PRECISION_STEP) {
if (includeDefaults || precisionStep != DEFAULT_PRECISION_STEP) {
builder.field("precision_step", precisionStep);
}
builder.field("format", dateTimeFormatter.format());
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java
View file
Open in desktop
Expand Up @@ -66,6 +66,7 @@
public class DoubleFieldMapper extends NumberFieldMapper<Double> {

public static final String CONTENT_TYPE = "double";
public static final int DEFAULT_PRECISION_STEP = LongFieldMapper.DEFAULT_PRECISION_STEP;

public static class Defaults extends NumberFieldMapper.Defaults {
public static final FieldType FIELD_TYPE = new FieldType(NumberFieldMapper.Defaults.FIELD_TYPE);
Expand All @@ -82,7 +83,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, DoubleFie
protected Double nullValue = Defaults.NULL_VALUE;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
}

Expand Down Expand Up @@ -348,7 +349,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || precisionStep != Defaults.PRECISION_STEP) {
if (includeDefaults || precisionStep != DEFAULT_PRECISION_STEP) {
builder.field("precision_step", precisionStep);
}
if (includeDefaults || nullValue != null) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java
View file
Open in desktop
Expand Up @@ -67,6 +67,7 @@
public class FloatFieldMapper extends NumberFieldMapper<Float> {

public static final String CONTENT_TYPE = "float";
public static final int DEFAULT_PRECISION_STEP = IntegerFieldMapper.DEFAULT_PRECISION_STEP;

public static class Defaults extends NumberFieldMapper.Defaults {
public static final FieldType FIELD_TYPE = new FieldType(NumberFieldMapper.Defaults.FIELD_TYPE);
Expand All @@ -83,7 +84,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, FloatFiel
protected Float nullValue = Defaults.NULL_VALUE;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
}

Expand Down Expand Up @@ -354,7 +355,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || precisionStep != Defaults.PRECISION_STEP) {
if (includeDefaults || precisionStep != DEFAULT_PRECISION_STEP) {
builder.field("precision_step", precisionStep);
}
if (includeDefaults || nullValue != null) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java
View file
Open in desktop
Expand Up @@ -63,6 +63,7 @@
public class IntegerFieldMapper extends NumberFieldMapper<Integer> {

public static final String CONTENT_TYPE = "integer";
public static final int DEFAULT_PRECISION_STEP = 8;

public static class Defaults extends NumberFieldMapper.Defaults {
public static final FieldType FIELD_TYPE = new FieldType(NumberFieldMapper.Defaults.FIELD_TYPE);
Expand All @@ -79,7 +80,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, IntegerFi
protected Integer nullValue = Defaults.NULL_VALUE;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
}

Expand Down Expand Up @@ -349,7 +350,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || precisionStep != Defaults.PRECISION_STEP) {
if (includeDefaults || precisionStep != DEFAULT_PRECISION_STEP) {
builder.field("precision_step", precisionStep);
}
if (includeDefaults || nullValue != null) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java
View file
Open in desktop
Expand Up @@ -63,6 +63,7 @@
public class LongFieldMapper extends NumberFieldMapper<Long> {

public static final String CONTENT_TYPE = "long";
public static final int DEFAULT_PRECISION_STEP = 16;

public static class Defaults extends NumberFieldMapper.Defaults {
public static final FieldType FIELD_TYPE = new FieldType(NumberFieldMapper.Defaults.FIELD_TYPE);
Expand All @@ -79,7 +80,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, LongField
protected Long nullValue = Defaults.NULL_VALUE;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
}

Expand Down Expand Up @@ -331,7 +332,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || precisionStep != Defaults.PRECISION_STEP) {
if (includeDefaults || precisionStep != DEFAULT_PRECISION_STEP) {
builder.field("precision_step", precisionStep);
}
if (includeDefaults || nullValue != null) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/elasticsearch/index/mapper/core/Murmur3FieldMapper.java
View file
Open in desktop
Expand Up @@ -50,7 +50,7 @@ public static class Defaults extends LongFieldMapper.Defaults {
public static class Builder extends NumberFieldMapper.Builder<Builder, Murmur3FieldMapper> {

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
builder.precisionStep(Integer.MAX_VALUE);
}
Expand Down
22 changes: 14 additions & 8 deletions src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java
View file
Open in desktop
Expand Up @@ -35,7 +35,6 @@
import org.apache.lucene.search.Query;
import org.apache.lucene.store.ByteArrayDataOutput;
import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.NumericUtils;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -64,7 +63,6 @@
public abstract class NumberFieldMapper<T extends Number> extends AbstractFieldMapper<T> implements AllFieldMapper.IncludeInAll {

public static class Defaults extends AbstractFieldMapper.Defaults {
public static final int PRECISION_STEP = NumericUtils.PRECISION_STEP_DEFAULT;

public static final FieldType FIELD_TYPE = new FieldType(AbstractFieldMapper.Defaults.FIELD_TYPE);

Expand All @@ -82,14 +80,15 @@ public static class Defaults extends AbstractFieldMapper.Defaults {

public abstract static class Builder<T extends Builder, Y extends NumberFieldMapper> extends AbstractFieldMapper.Builder<T, Y> {

protected int precisionStep = Defaults.PRECISION_STEP;
protected int precisionStep;

private Boolean ignoreMalformed;

private Boolean coerce;

public Builder(String name, FieldType fieldType) {
public Builder(String name, FieldType fieldType, int defaultPrecisionStep) {
super(name, fieldType);
this.precisionStep = defaultPrecisionStep;
}

public T precisionStep(int precisionStep) {
Expand Down Expand Up @@ -157,6 +156,13 @@ protected NumericTokenStream initialValue() {
return new NumericTokenStream(8);
}
};

private static ThreadLocal<NumericTokenStream> tokenStream16 = new ThreadLocal<NumericTokenStream>() {
@Override
protected NumericTokenStream initialValue() {
return new NumericTokenStream(16);
}
};

private static ThreadLocal<NumericTokenStream> tokenStreamMax = new ThreadLocal<NumericTokenStream>() {
@Override
Expand Down Expand Up @@ -369,11 +375,11 @@ public void close() {
protected NumericTokenStream popCachedStream() {
if (precisionStep == 4) {
return tokenStream4.get();
}
if (precisionStep == 8) {
} else if (precisionStep == 8) {
return tokenStream8.get();
}
if (precisionStep == Integer.MAX_VALUE) {
} else if (precisionStep == 16) {
return tokenStream16.get();
} else if (precisionStep == Integer.MAX_VALUE) {
return tokenStreamMax.get();
}
return tokenStream.get();
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java
View file
Open in desktop
Expand Up @@ -64,6 +64,7 @@
public class ShortFieldMapper extends NumberFieldMapper<Short> {

public static final String CONTENT_TYPE = "short";
public static final int DEFAULT_PRECISION_STEP = 8;

public static class Defaults extends NumberFieldMapper.Defaults {
public static final FieldType FIELD_TYPE = new FieldType(NumberFieldMapper.Defaults.FIELD_TYPE);
Expand All @@ -80,7 +81,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, ShortFiel
protected Short nullValue = Defaults.NULL_VALUE;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
}

Expand Down Expand Up @@ -346,7 +347,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || precisionStep != Defaults.PRECISION_STEP) {
if (includeDefaults || precisionStep != DEFAULT_PRECISION_STEP) {
builder.field("precision_step", precisionStep);
}
if (includeDefaults || nullValue != null) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/elasticsearch/index/mapper/core/TokenCountFieldMapper.java
View file
Open in desktop
Expand Up @@ -57,7 +57,7 @@ public static class Builder extends NumberFieldMapper.Builder<Builder, TokenCoun
private NamedAnalyzer analyzer;

public Builder(String name) {
super(name, new FieldType(Defaults.FIELD_TYPE));
super(name, new FieldType(Defaults.FIELD_TYPE), DEFAULT_PRECISION_STEP);
builder = this;
}

Expand Down