[GitHub] lucenenet issue #174: Ported over Lucene.Net.Spatial
Github user eladmarg commented on the issue: https://github.com/apache/lucenenet/pull/174 I think I will write a tool for that, this will be good also for the next version of port. @conniey, is it possible to ask the guys from TFS if they have the diff API? they already handled this kind of merging stuff. maybe they can provide this API with a couple of hours. the logic is pretty simple, 1. diff between the java versions 2. then take each diff part and try to merge it to the C# version in the correct place / file. this can be also added as comment with TODO label, and even after the auto conversion tool (java2cs) of the code snippet. I'm currently packed, so I'll try to get into it during the weekend. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet issue #174: Ported over Lucene.Net.Spatial
Github user eladmarg commented on the issue: https://github.com/apache/lucenenet/pull/174 Hi, what should be done here? I can try to re-port the spatial4j will this solve the issue? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet issue #188: Fixed 64 Failing Facet Tests and Finished Facet Implem...
Github user eladmarg commented on the issue: https://github.com/apache/lucenenet/pull/188 there are few files of the LurchTable, so all we need is @csharptest permission, and then we can just copy them to the solution and fork the relevant parts. I think if we'll contact @csharptest, he won't have any opposition. The LurchTable is a tested and working solution already, so i think its better to use what's working. @synhershko is it possible to email @csharptest ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet issue #188: Fixed 64 Failing Facet Tests and Finished Facet Implem...
Github user eladmarg commented on the issue:
https://github.com/apache/lucenenet/pull/188
Hi,
just found a great implementation which does exactly what is expected
(truly same as java)
please take a look at :
[CSharpTest.Net.Collections](https://github.com/csharptest/CSharpTest.Net.Collections),
there is a [nuget](https://www.nuget.org/packages/CSharpTest.Net.Collections/)
package as well.
at the constructor use:
`new LurchTable<TKey, TValue>(LurchTableOrder.Access, capacity,
EqualityComparer.Default)
`
the lucene get method expect for default(TValue) or null, but the
LurchTable throw exception,
so just wrap the get method with:
` TValue result;
if (!lru.TryGetValue(key, out result))
{
return default(TValue);
}
return result;
`
all tests working, (include the LruHashMap), performance much faster.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #188: Fixed 64 Failing Facet Tests and Finished Facet Implem...
Github user eladmarg commented on the issue: https://github.com/apache/lucenenet/pull/188 by the way, another option is to port the java equivalent [LinkedHashMap](http://developer.classpath.org/doc/java/util/LinkedHashMap-source.html) derived from HashSet and just implement the Access method on Gets. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request #188: Fixed 64 Failing Facet Tests and Finished Facet...
Github user eladmarg commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/188#discussion_r81025984
--- Diff: src/Lucene.Net.Facet/Taxonomy/LRUHashMap.cs ---
@@ -100,19 +120,25 @@ public bool Put(TKey key, TValue value)
timestamp = GetTimestamp()
};
// We have added a new item, so we may need to remove
the eldest
-if (cache.Count > Capacity)
+if (cache.Count > capacity)
{
// Remove the eldest item (lowest timestamp) from
the cache
cache.Remove(cache.OrderBy(x =>
x.Value.timestamp).First().Key);
}
}
+
+return true;
+}
+finally
+{
+syncLock.ExitWriteLock();
}
-return true;
}
public TValue Get(TKey key)
{
-lock (syncLock)
+syncLock.EnterWriteLock();
--- End diff --
I can confirm that after replacing to ConcurrentDictionary implementation,
same all tests pass.
(Except TestLRUHashMap, but we can ignore it)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #188: Fixed 64 Failing Facet Tests and Finished Facet...
Github user eladmarg commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/188#discussion_r81009940
--- Diff: src/Lucene.Net.Facet/Taxonomy/LRUHashMap.cs ---
@@ -100,19 +120,25 @@ public bool Put(TKey key, TValue value)
timestamp = GetTimestamp()
};
// We have added a new item, so we may need to remove
the eldest
-if (cache.Count > Capacity)
+if (cache.Count > capacity)
{
// Remove the eldest item (lowest timestamp) from
the cache
cache.Remove(cache.OrderBy(x =>
x.Value.timestamp).First().Key);
}
}
+
+return true;
+}
+finally
+{
+syncLock.ExitWriteLock();
}
-return true;
}
public TValue Get(TKey key)
{
-lock (syncLock)
+syncLock.EnterWriteLock();
--- End diff --
I'm not sure if it's worth the pay of price of thread safe here,
Generally, the main purpose of LRUCache is to delete the old entries, we
want to store ~10K items, we don't really care if we removed the ones that has
been used 2 ticks later.
so the order is nice to have but not a must, this is a cache so the main
goal is performance.
here is mine implementation, you can take a look.
https://gist.github.com/eladmarg/8d4a7f7a43a36c35d488e99475a101d9
yes, there is a chance that the Lru tests will fail, but we shouldn't care.
this is of course just a recommendation, we should get the version out and
then think about performance.
Thank You!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #88: fix lru hash map
Github user eladmarg closed the pull request at: https://github.com/apache/lucenenet/pull/88 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet issue #188: Fixed 64 Failing Facet Tests and Finished Facet Implem...
Github user eladmarg commented on the issue: https://github.com/apache/lucenenet/pull/188 Fantastic! this is awesome, thank you so much! @synhershko please merge Again, thanks a lot! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: Removing Apache.NMS dependency.
Github user eladmarg commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/168#discussion_r60835981
--- Diff: src/Lucene.Net.Core/Support/AtomicObject.cs ---
@@ -0,0 +1,39 @@
+using System;
+using System.Threading;
+
+namespace Lucene.Net.Support
+{
+public class AtomicObject where T : class
+{
+private T _value;
+private ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
+
+public AtomicObject() : this(default(T))
+{ }
+
+public AtomicObject(T initial)
+{
+_value = initial;
+}
+
+public T Value
+{
+get
+{
+try
+{
+_lock.EnterReadLock();
+return _value;
+}
+finally
+{
+_lock.ExitReadLock();
+}
+}
+set
+{
+Interlocked.Exchange(ref _value, value);
--- End diff --
Great job :+1:
just for safety, I would add
`
try
{
_lock.EnterWriteLock();
Interlocked.Exchange(ref _value, value);
}
finally
{
_lock.ExitWriteLock();
}`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: synchronize access to underlying file stre...
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/147#issuecomment-102937125 @synhershko , Thanks. I think this solution also won't solve the problem. instead, the solution should be many FileStream objects, which require a big refactor. However, I agree that first priority is to get a stable release and only then, performance optimization --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: synchronize access to underlying file stre...
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/147#issuecomment-102824774 Great catch! However, I would just replace the lock with ReaderWriterLockSlim and EnterReadLock. this might be performance critical section. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: synchronize access to underlying file stre...
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/147#issuecomment-102838744 while the FileStream seek is moving to another pointer, another thread will not read the correct position. The correct way to do it is by async take a look: https://github.com/eladmarg/lucene.net/commit/7ea8f870ee3b3e9fb3512dee0fbee67b69a6da86 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: Fix on the PQ.
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/92#issuecomment-74473922 maybe it is possible to assign default comparer T as default value --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: fix lru hash map
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/88#issuecomment-74419443 can it be merged? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: Fix on the PQ.
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/92#issuecomment-74150045 @guidotag I Agree with you, passing comparer seems more naturally, more generic and more readable code. We're at .net, so we should follow the attitude. how many files involved with this change? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: Fix for TestIndexableField.TestArbitraryFi...
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/96#issuecomment-73934712 this is correct fix, no need to change bytesref, this is on purpose. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: fix lru hashmap
Github user eladmarg closed the pull request at: https://github.com/apache/lucenenet/pull/87 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: fix lru hashmap
GitHub user eladmarg opened a pull request: https://github.com/apache/lucenenet/pull/87 fix lru hashmap previous implementation was wrong, this one is same as the java one. You can merge this pull request into a Git repository by running: $ git pull https://github.com/eladmarg/lucene.net master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucenenet/pull/87.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #87 commit e1bc6f8200aca3ab61ad4a64f07dde8a5630be6b Author: eladmarg eladm...@gmail.com Date: 2014-11-14T22:45:39Z fix for dictionary getter (assume nulls in results) commit cd0ddd51c79810e3032434512032e3ad7861af60 Author: eladmarg eladm...@gmail.com Date: 2015-02-08T14:40:21Z Merge branch 'master' of https://github.com/apache/lucene.net commit f66418730d86a4e58a24634f28f80e784135e7d2 Author: eladmarg eladm...@gmail.com Date: 2015-02-08T14:41:44Z fix lru hash map --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: fix lru hash map
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/88#issuecomment-73464544 my previous implementation was wrong, this is not implemented from scratch, and its working. (tests are passing) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: Expressions with some failing tests
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/64#issuecomment-71873706 great job! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] lucenenet pull request: Merge logic fixes
Github user eladmarg commented on the pull request: https://github.com/apache/lucenenet/pull/59#issuecomment-71432169 awesome! I knew there is a bug in the segments, but couldn't find it, thanks :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
