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

Feature/combine mapper filtersearch #2643

Merged
merged 13 commits into from
Aug 8, 2022
Merged

Feature/combine mapper filtersearch #2643

merged 13 commits into from
Aug 8, 2022

Conversation

thoniTUB
Copy link
Collaborator

@thoniTUB thoniTUB commented Aug 2, 2022

No description provided.

@thoniTUB thoniTUB requested a review from awildturtok August 2, 2022 16:10
awildturtok
awildturtok requested changes Aug 3, 2022
View reviewed changes
backend/src/main/java/com/bakdata/conquery/apiv1/query/FilterConfig.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/datasets/concepts/Searchable.java Show resolved Hide resolved
...c/main/java/com/bakdata/conquery/models/datasets/concepts/filters/specific/SelectFilter.java Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/AbstractIndexKey.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/FEValueIndex.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/FEValueIndex.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/IndexService.java Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/models/index/IndexService.java Outdated Show resolved Hide resolved
backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java Outdated Show resolved Hide resolved
backend/src/test/java/com/bakdata/conquery/integration/IntegrationTests.java Show resolved Hide resolved
@thoniTUB thoniTUB requested a review from awildturtok August 4, 2022 07:10
@thoniTUB thoniTUB marked this pull request as ready for review August 4, 2022 07:10
awildturtok
awildturtok requested changes Aug 4, 2022
View reviewed changes
backend/src/main/java/com/bakdata/conquery/apiv1/FilterSearch.java Outdated
@@ -126,38 +125,28 @@ public void updateSearch() {
for (Searchable searchable : collectedSearchables) {

service.submit(() -> {
final Stream<FEValue> values = searchable.getSearchValues(getParserConfig(), storage);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mir ist gestern aufgefallen, dass sich der Job nicht canceln lässt, kannst du das heir in einen separaten Job heben und dann eine cancel methode implementieren? Es reicht ein shutdownNow zu machen in der join-loop und dann zu exiten wenn der rest durchgelaufen ist (vlt auch ein clear?) das hätte mir gestern einiges an zeit gespart :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Der Kommentar ließ sich leider nicht weiter oben machen, ich meine für Zeile 92/93

backend/src/main/java/com/bakdata/conquery/apiv1/FilterTemplate.java
// We inject the service as a non-final property so, jackson will never try to create a serializer for it (in contrast to constructor injection)
@JsonIgnore
@JacksonInject(useInput = OptBoolean.FALSE)
@NonNull
Copy link
Collaborator

Choose a reason for hiding this comment

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

NotNull oder NonNul?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NonNull, auf einem Shard darf es null sein, es sollte aber nie explizit auf null gesetzt werden.

backend/src/main/java/com/bakdata/conquery/models/index/Index.java

import java.util.Map;

public interface Index<T extends IndexKey<? extends Index<T>>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

bisschen komisch, dass ein Index nur put hat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Es hatte erst auch ein get, aber das brauchte ich letztendlich nicht

backend/src/main/java/com/bakdata/conquery/models/index/IndexService.java Outdated
}
}
catch (IOException ioException) {
log.warn("Failed to open {}", key.getCsv(), ioException);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log.warn("Failed to open {}", key.getCsv(), ioException);
log.warn("Failed to open `{}`", key.getCsv(), ioException);

Damit man sehen kann, ob es evtl Whitespace Error sind :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ich benutze jetzt
String externalValueCleaned = CharMatcher.whitespace().trimAndCollapseFrom(externalValue, ' ');

backend/src/main/java/com/bakdata/conquery/models/index/IndexService.java Outdated
final String externalValue = substitutor.replace(externalTemplate);

// Clean up the substitution by removing repeated white spaces
String externalValueCleaned = externalValue.replaceAll("\\s+", " ");
Copy link
Collaborator

Choose a reason for hiding this comment

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

kannst du das Regex schon woanders erzeugen? So wird das im HotLoop erzeugt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ich benutze jetzt
String externalValueCleaned = CharMatcher.whitespace().trimAndCollapseFrom(externalValue, ' ');

awildturtok
awildturtok requested changes Aug 4, 2022
View reviewed changes
backend/src/main/java/com/bakdata/conquery/models/query/FilterSearch.java

@Slf4j
@Data
public class FilterSearch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Warum ist sie jetzt in query?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nach api passt sie nicht so wirklich, nach dw müsste sie eigentlich in ein core package

backend/src/test/java/com/bakdata/conquery/MatcherTest.java Outdated
Comment on lines 8 to 13
public class MatcherTest {
@Test
void test() {

log.info("result {}", CharMatcher.whitespace().trimAndCollapseFrom("hekd dskf d dkfjdk", ' '));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

💣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

upps mache ich noch wieder raus

thoniTUB and others added 3 commits August 5, 2022 17:53
@thoniTUB
remove matcher test
a68c3bd
@thoniTUB
Merge branch 'develop' into feature/combine-mapper-filtersearch
e1ff669 
# Conflicts:
#	backend/src/test/resources/tests/endpoints/adminEndpointInfo.json
#	docs/Table JSONs.md
@github-actions
Update AutoDoc
a60f002
@thoniTUB thoniTUB merged commit e854769 into develop Aug 8, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/combine-mapper-filtersearch branch August 8, 2022 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awildturtok awildturtok awildturtok requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thoniTUB @awildturtok