Skip to content

[Issue #31] (Team 4) Enabling positional indexing in Lucene for TEXT type#103

Merged
akshaybetala merged 11 commits into
masterfrom
team4-phrase-operator
May 18, 2016
Merged

[Issue #31] (Team 4) Enabling positional indexing in Lucene for TEXT type#103
akshaybetala merged 11 commits into
masterfrom
team4-phrase-operator

Conversation

@akshaybetala
Copy link
Copy Markdown
Contributor

@akshaybetala akshaybetala commented May 17, 2016

Previously a TEXT field doesn't enable positional indexing. In this PR we enable positional indexing in Lucene so that we can return information about character offsets and token offsets.

@akshaybetala
Copy link
Copy Markdown
Contributor Author

@sandeepreddy602 @chenlica Please review.

fieldName, (String) fieldValue, Store.YES);
break;

org.apache.lucene.document.FieldType luceneFieldType = new org.apache.lucene.document.FieldType();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add comments to the codebase: "By default we enable positional indexing in Lucene so that we can return information about character offsets and token offsets.""

@chenlica chenlica changed the title Enabling Term Vectors [Issue #31] (Team 4) Enabling positional indexing in Lucene for TEXT type May 17, 2016
@chenlica
Copy link
Copy Markdown
Contributor

I left a few minor comments. It will be good if @sandeepreddy602 can also review it quickly. Then you can do the merge.

@akshaybetala
Copy link
Copy Markdown
Contributor Author

@chenlica @sandeepreddy602 @prakul Please Review.

this.end = end;
this.key = key;
this.value = value;
this.tokenOffset = -1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Give "-1" to a meaning constant such as "INVALID_TOKEN_OFFSET"?

@chenlica
Copy link
Copy Markdown
Contributor

Added @zuozhi and @rajesh9625 in case they are interested to review as well.

if (other.list != null)
return false;
} else if (!list.equals(other.list))
} else if (!list.containsAll(other.list))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why don't we use equals()?

Copy link
Copy Markdown
Contributor Author

@akshaybetala akshaybetala May 17, 2016

Choose a reason for hiding this comment

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

Because even though the list has same number of elements, with same values, equals return false, where as containsAll returns True

other.list.containsAll(list) Returns True;
list.containsAll(others.list) Returns True;
other.list.equals(list) Returns False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it because equals() cares about order, why we don't? If so, then the name "list" is not accurate. Should we rename it to "SetField"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why is the "list" name not accurate?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am still not following. Why "other.list.equals(list) Returns False"?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants