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.

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

decrease earliest epoch value of query xml by 3 hours query XML #415

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

elliVM
Copy link
Contributor

@elliVM elliVM commented Nov 14, 2024

Workaround to decrease earliest epoch value given to query XML by 3 hours to work with pth_06 that uses helsinki timezone.

@elliVM
Copy link
Contributor Author

elliVM commented Nov 14, 2024

For issues #83 and #401

@kortemik
Copy link
Member

please rebase

@elliVM elliVM force-pushed the timestamp-epoch-minus-three-hours branch from 1551c09 to bfeceed Compare November 15, 2024 09:39
@elliVM
Copy link
Contributor Author

elliVM commented Nov 15, 2024

Rebased

@eemhu
Copy link
Contributor

eemhu commented Nov 15, 2024

I think you also need to change it for the produced XML and spark column to have an effect


if (tq.isStartTime()) {
startTime = tq.epoch();
final TimeQualifierInterface timeQualifierInterface = new TimeQualifier(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename TimeQualiferInterface to TimeQualifier, and the implementing class to TimeQualifierImpl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@elliVM elliVM requested a review from 51-code November 15, 2024 11:33
@elliVM elliVM marked this pull request as draft November 15, 2024 11:34
@elliVM elliVM requested a review from 51-code November 18, 2024 13:06
Copy link

@51-code 51-code left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@eemhu eemhu left a comment

Choose a reason for hiding this comment

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

LGTM, but should EqualsVerifier be added to pth-10 as well for hashCode tests?

@kortemik
Copy link
Member

LGTM, but should EqualsVerifier be added to pth-10 as well for hashCode tests?

yes, please add @elliVM

import java.util.Objects;

/** Decreases the amount from qualifier epoch value */
public final class DecreasedEpochValue implements TimeQualifier {
Copy link
Member

Choose a reason for hiding this comment

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

please rename to DecreasedEpochXmlElementValue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed


import java.util.Objects;

/** Decreases the amount from qualifier epoch value */
Copy link
Member

Choose a reason for hiding this comment

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

please add comment that this is a temporary workaround for the pth_06 database schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added explanatory comment in javadoc


@Override
public long epoch() {
return origin.epoch() - decreaseAmount;
Copy link
Member

Choose a reason for hiding this comment

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

only XML is to be decremented, the datasource code extracts proper timestamps from the data itself, only the search uses local timestamps incorrectly and that's controlled by the xml value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change to only change value inside XML element, otherwise uses origins methods

@elliVM
Copy link
Contributor Author

elliVM commented Nov 20, 2024

Added EqualsVerifier library and contract test.

@elliVM elliVM requested a review from kortemik November 20, 2024 10:48
Copy link
Contributor

@eemhu eemhu left a comment

Choose a reason for hiding this comment

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

LGTM

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