Update _get_is_datetime constraint logic to use the metadata

[fealho]

CU-86aytb6w0, Resolve #1692

[sdv-team]

Task linked: CU-86aytb6w0 SDV - InvalidDataError for Inequality constraint (even though data is valid) #1692

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f25037b) 97.07% compared to head (48ac779) 97.14%.

❗ Current head 48ac779 differs from pull request most recent head f360d98. Consider uploading reports for the commit f360d98 to get more accurate results

Files	Patch %	Lines
sdv/constraints/tabular.py	95.45%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                       Coverage Diff                       @@
##           issue-1692-inequality-error    #1732      +/-   ##
===============================================================
+ Coverage                        97.07%   97.14%   +0.07%     
===============================================================
  Files                               48       48              
  Lines                             4506     4522      +16     
===============================================================
+ Hits                              4374     4393      +19     
+ Misses                             132      129       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[amontanez24]

Part of the issue is to use the provided datetime format if applicable. I think the code in #1692 would still fail right now. Either way, can we add an integration test that checks that the datetime format is indeed being used to convert weird datetime columns during the constraints?

[fealho]

Part of the issue is to use the provided datetime format if applicable. I think the code in #1692 would still fail right now. Either way, can we add an integration test that checks that the datetime format is indeed being used to convert weird datetime columns during the constraints?

@amontanez24 I already tested that specific integration test in the other PR, but I can add an additional test for the datetime format as well

[amontanez24]

Part of the issue is to use the provided datetime format if applicable. I think the code in #1692 would still fail right now. Either way, can we add an integration test that checks that the datetime format is indeed being used to convert weird datetime columns during the constraints?

@amontanez24 I already tested that specific integration test in the other PR, but I can add an additional test for the datetime format as well

I thought part of the problem was that we weren't converting the datetime properly because we didn't use the format but I guess I remembered it wrong. Idk if that's still a potential problem that could happen with oddly formed datetime strings though

[fealho]

Part of the issue is to use the provided datetime format if applicable. I think the code in #1692 would still fail right now. Either way, can we add an integration test that checks that the datetime format is indeed being used to convert weird datetime columns during the constraints?

@amontanez24 I already tested that specific integration test in the other PR, but I can add an additional test for the datetime format as well

I thought part of the problem was that we weren't converting the datetime properly because we didn't use the format but I guess I remembered it wrong. Idk if that's still a potential problem that could happen with oddly formed datetime strings though

@amontanez24 The issue 1692 is not really related to the datetime_format, it was just misdiagnosed as such in the beginning, where the true issue was misidentifying the datetime column sdtype.

To assuage your concerns about the datatime_format, we already verify it is valid. fit calls validate, which in turn calls validate_datetime_format:

    pandas_datetime_format = datetime_format.replace('%-', '%')
    datetime_column = pd.to_datetime(
        column,
        errors='coerce',
        format=pandas_datetime_format
    )
    valid = pd.isna(column) | ~pd.isna(datetime_column)

    return set(column[~valid])

If datetime_format doesn't match the column, an error will be raised during the validation step.

[pvk-developer]

LGTM!

[amontanez24]

Part of the issue is to use the provided datetime format if applicable. I think the code in #1692 would still fail right now. Either way, can we add an integration test that checks that the datetime format is indeed being used to convert weird datetime columns during the constraints?

@amontanez24 I already tested that specific integration test in the other PR, but I can add an additional test for the datetime format as well

I thought part of the problem was that we weren't converting the datetime properly because we didn't use the format but I guess I remembered it wrong. Idk if that's still a potential problem that could happen with oddly formed datetime strings though

@amontanez24 The issue 1692 is not really related to the datetime_format, it was just misdiagnosed as such in the beginning, where the true issue was misidentifying the datetime column sdtype.

To assuage your concerns about the datatime_format, we already verify it is valid. fit calls validate, which in turn calls validate_datetime_format:

    pandas_datetime_format = datetime_format.replace('%-', '%')
    datetime_column = pd.to_datetime(
        column,
        errors='coerce',
        format=pandas_datetime_format
    )
    valid = pd.isna(column) | ~pd.isna(datetime_column)

    return set(column[~valid])

If datetime_format doesn't match the column, an error will be raised during the validation step.

I see. No, I'm not concerned about the format being valid. I'm saying that sometimes when converting a string column to datetime with the pd.to_datetime() function, it fails if you don't provide the format. So if we make that call at all in the constraints, I think we should pass the format there

[amontanez24]

I think this looks good!

[pvk-developer]

LGTM! 👍🏻