Autogenerated ShadowMesh uses same LoD level geometry as source Mesh

Godot version:
Godot 4.x
commit b4b7c97d3839170180e4f3f7c2bd3179b374057a
OS/device including version:
Win 10 64x
Issue description:
After testing the new ShadowMeshes in Godot 4.x and comparing the new feature with manual ShadowMeshes created for Godot 3.x I couldn't help but feel very disappointed by the performance (~80% less than Godot 3.x).
So I looked at the shadowmesh source code and I consider this an implementation bug and not a missing feature for a proposal.
Autogenerated shadowmeshes in Godot 4.x are matched to the same source mesh LoD level while they should be matched 1-3 LoD levels lower. A shadow imposter mesh created manual by an artist always starts at least 1-2 LoD levels below the visible LoD mesh but never at the same level. A typical example would be a characters hair mesh, starting at 150.000 vertex. The shadow looks the same with a LOD3 mesh that has only 5.000 vertex cause the outside mesh shape stays the same and this is enough for shadow casting on solid objects. In the current implementation the shadow casting mesh has a 145.000 vertex overhead that does nothing for the endresult and just consumes performance.
Due to this as mentioned in #45452 pull request there is currently only a little performance gain with shadow imposters while it should be a considerable one.
I suggest that the autogenerated shadow imposters should be generated and matched with LOD2 or LOD3 meshes by default to fix this issue and give the expected performance boost from shadow imposters.
There is no reason to ever use the same mesh geometry detail for a shadow-only casting object except for edgecases like meshes mixed with semi-transparent surfaces and very lowpoly meshes (that don't need shadow imposters to begin with). A Shadowmesh LoD selection option in the new ResourceImporter would be welcome to solve those edgecases.
Steps to reproduce:
Minimal reproduction project:

cc @fire

Eh, #45452 isn't about generating Shadow Mesh LODs...
as @reduz said:

-When importing, a vertex-only version of the mesh is created.
-This version is used when rendering shadows, and improves performance by reducing bandwidth

This doesn't Mean that the Generated Mesh is supposed to be a LOD mesh. If I understand what the Optimization is doing (Not a Rendering Expert) It's Giving The Renderer Only Verts to render the shadow, instead of triangles, because with triangles it wouldn't be uncommon for a vertex to be a part of multiple triangles, resulting in multiple copies of the same vertex, which increases bandwidth usage, and thus reduces performance.

@mrjustaguy is very close, ShadowMeshes are not the same thing as ShadowImposters. ShadowMeshes are a copy of the mesh that contain vertex positions and indices only (rather than including normals, tangents, UVs, etc.). ShadowMeshes contain all the same LODs as the regular mesh.
I think there needs to be a proposal for adding a "Shadow LOD bias" parameter somewhere that shifts which LOD to use for the shadow meshes. Perhaps an import setting?

Could be a Mesh Instance Setting, like LOD bias is.

@mrjustaguy Yep you are right, the linked pullrequest is not directly the same but I thought it is involved.
@clayjohn thanks for the more detailed information.
I think one issue is that the terms ShadowMesh and ShadowImposter are mixed all the time by people that have no technical render background. I see experiences 3D artists use both terms all the time when they mean ShadowImposters and the user interface in Godot 4.x uses the term "Generate -> Shadow Meshes" right next to LOD generation.
I think a pulldown option in the new Advanced Import Settings menu with all available mesh LOD levels could work to pick the starting mesh detail for the shadow meshes / imposters.
Something like "ShadowMesh Base LOD Level".
Set to default LOD1 or LOD2 if available on the mesh.

Currently a Workaround would be (if Cast Shadow property, under geometry, worked, which it doesn't seem to do in current G4.0) is to make 2 mesh instances, a Mesh that is visible but doesn't cast shadows, and a shadow mesh that is set to just cast shadows, with a lower LOD Bias.


Incorrect API key provided

Hello guys, I have faced with issue that's my API key incorrect. I've tried to create a new one but still have the same problem. Previously everything was ok. I have money on my OpenAI acc so it's not related to an empty balance or something like that.
Any idea why I have it ?
APIErrorResponse(error: OpenAI.APIError(message: "Incorrect API key provided: "sk-proj**********************************************". You can find your API key at https://platform.openai.com/account/api-keys.", type: "invalid_request_error", param: nil, code: Optional("invalid_api_key")))
OpenAI 0.2.6 version (SPM package)

OpenAI recently rolled out per project API keys with the following format:
sk-proj-abcdefghijklmnopqrstuvwxyz1234567890abcdef

Although this article doesn't seem to mention this specific format change you can read about it here:
https://help.openai.com/en/articles/9186755-managing-your-work-in-the-api-platform-with-projects
You might be able to create a your own Project API key to verify here:
https://platform.openai.com/api-keys
This library likely does a format check to confirm that the key is the expected classic key format so that the user didn't accidentally type or paste the wrong text somehow.
sk-abcdefghijklmnopqrstuvwxyz1234567890abcdef

OpenAI recently rolled out per project API keys with the following format:

sk-proj-abcdefghijklmnopqrstuvwxyz1234567890abcdef

Although this article doesn't seem to mention this specific format change you can read about it here:
https://help.openai.com/en/articles/9186755-managing-your-work-in-the-api-platform-with-projects
You might be able to create a your own Project API key to verify here:
https://platform.openai.com/api-keys
This library likely does a format check to confirm that the key is the expected classic key format so that the user didn't accidentally type or paste the wrong text somehow.

sk-abcdefghijklmnopqrstuvwxyz1234567890abcdef

I know that, even with new key (project key) it does not work. That's why I'm asking here about that)

Hey bro can u provide me open ai api key for testing out a project u can deactivate after 1 hour plzz my credits are already expired help me out


helm dependency build fails when repo URL contains escaped

With the following : Chart.yaml:
apiVersion: v2
name: "gatekeeper"
version: 0.0.1
dependencies:
  - name: gatekeeper
    repository: https://gitlab.example.org/api/v4/projects/external-packages%2fgatekeeper/packages/helm/stable/
    version: 3.4.0

(the ending / is to workaround #9764).
helm dependency build failed exit status 1:
Error: could not download https://gitlab.example.org/api/v4/projects/external-packages/gatekeeper/packages/helm/stable/charts/gatekeeper-3.4.0.tgz:
failed to fetch https://gitlab.example.org/api/v4/projects/external-packages/gatekeeper/packages/helm/stable/charts/gatekeeper-3.4.0.tgz

As you can see, %2 has been replaced by /.

@sathieu I tried to reproduce the issue using the latest code from main and it looks like it is now working. Here is how I tested:
$ cat pr-9822/Chart.yaml

apiVersion: v2
name: pr-9822
description: A Helm chart for Kubernetes

[...]

dependencies:
  - name: kubitus-ingress-operator
    repository: https://gitlab.com/api/v4/projects/kubitus-project%2Fkubitus-ingress-operator/packages/helm/main
    version: 0.1.0-20210728154159-bdb9fe13

 $ helm dep build pr-9822

Getting updates for unmanaged Helm repositories...
...Successfully got an update from the "https://gitlab.com/api/v4/projects/kubitus-project%2Fkubitus-ingress-operator/packages/helm/main" chart repository
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "prometheus-community" chart repository
...Successfully got an update from the "iks-charts" chart repository
...Successfully got an update from the "bitnami" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading kubitus-ingress-operator from repo https://gitlab.com/api/v4/projects/kubitus-project%2Fkubitus-ingress-operator/packages/helm/main
Deleting outdated charts

A fix may have been applied in the meantime?

@hickeyma Yes, probably fixed by another commit from me (d506314abfb5d21419df8c7e7e68012379db2354), which is in v3.6.3.
unfortunately, ArgoCD has not updated it yet (https://github.com/argoproj/argo-cd/pull/6654).

@sathieu Do you mind testing this against v3.6.3 and making sure it is working as expected? If it is then we should close this issue as already fixed.


Group chat history by month/year

The list of previous chats gets pretty long pretty quickly. It would be nice if old chats were automatically grouped in some convenient way, perhaps by week for the last few weeks, by month for the last few months before that and finally by year for anything older than maybe 6 months?

Refactor getTimeSince
Expandable groupings


Home page UI

the size of fonts on home page gives a bad user experience  and even the padding to div is more that makes it congested one doesn't look good to the eyes.
( font size in div is too small )

Please assign me this issue

i can fix this issue?

Hi shivpratikhande,
I have recent experience with web development and am enthusiastic about contributing to Hacktoberfest. Please assign me this issue. Eager to collaborate and learn. Thanks!

please assign this issue for me

could you please assign this issue to me ?


[Cherry-Pick] PWN-8260 - added retry logic for timeout errors during getting swap routes

Origin: #1682

2.7.0


H2O implementation

In the update_kv function of H2OKVCluster class, I see this code.
attn_weights = torch.matmul(query_states[..., -self.window_size:, :], key_states.transpose(2, 3)) / math.sqrt(head_dim)

As far as I know there is no concept of window in H2O. Shouldn't the entire query_states matrix be considered for attn_weights computation? Why are you only snipping out the window_size part from the query states to be considered for matrix multiplication here?

Thank you for pointing out!
The current code is inconsistent with standard H2O as you mentioned. We have tested performance with or without the entire query_state to calculate the attention. We found that this has very limited influence on the performance. We will address this inconsistency in the updated code.


0.2.5

Linter check found the following problems:
The following problems have been found:
ERROR: /tmp/abs_c3g7au4d7x/clone/recipe/meta.yaml:19: remove_python_pinning: On arch specific packages, python deps should not be constrained.
ERROR: /tmp/abs_c3g7au4d7x/clone/recipe/meta.yaml:22: host_section_needs_exact_pinnings: Packages in host must have exact version pinnings, except python build tools.
ERROR: /tmp/abs_c3g7au4d7x/clone/recipe/meta.yaml:26: remove_python_pinning: On arch specific packages, python deps should not be constrained.
ERROR: /tmp/abs_c3g7au4d7x/clone/recipe/meta.yaml:30: has_imports_and_run_test_py: Imports and python test file cannot coexist.
WARNING: /tmp/abs_c3g7au4d7x/clone/recipe/meta.yaml:44: missing_description: The recipe is missing a description
ERROR: build.sh:7: pip_install_args: pip install should be run with --no-deps.
Errors were found

Linter check found the following problems:
The following problems have been found:
ERROR: /tmp/abs_dbtc8xhdgj/clone/recipe/meta.yaml:23: host_section_needs_exact_pinnings: Packages in host must have exact version pinnings, except python build tools.
Errors were found

Linter check found the following problems:
The following problems have been found:
ERROR: /tmp/abs_f2b8i_2o3w/clone/recipe/meta.yaml:23: host_section_needs_exact_pinnings: Packages in host must have exact version pinnings, except python build tools.
Errors were found

Linter check found the following problems:
The following problems have been found:
ERROR: /tmp/abs_538chgtsvf/clone/recipe/meta.yaml:23: host_section_needs_exact_pinnings: Packages in host must have exact version pinnings, except python build tools.
Errors were found


Are multiple vaults allowed?

I don't know if this is a documentation issue or a  bug, so I'll ask. I have a vault registered using Microsoft.PowerShell.SecretStore. I then tried to create a second vault.
 Register-SecretVault -Name demo -Description "test vault" -ModuleName microsoft.powershell.secretstore

Get-SecretVault shows it. I have nothing in the vault but if I run Get-SecretInfo, I see all the entries from my default vault, even though the vault name shows the new demo vault. Are we limited to one vault per user of a given module? If so, that needs to be clearer in the documentation.

Clearly, there is some sort of interaction happening.

SecretManagement allows an extension vault to be registered multiple times, because it is possible that an extension vault may support different contexts via the registration VaultParameters.  However, the SecretStore vault currently always operates in the logged on user scope (see SecretStore documentation: https://github.com/powershell/secretstore).
So registering SecretStore multiple times with different names just results in duplication of the same store, and doesn't make sense.
But we decided to allow this for maximum flexibility.

Ok then. Again, it would help to have this clearer in the documentation. The bottom line is that a user can only have 1 microsoft.powershell.secretstore vault per user scope.  That's too bad because I can imagine a situation where I might want 2 stores based on the default Microsoft.PowerShell.SecretStore.  Or are you expecting that people will really be using an extension vault like LastPass?

Thanks @jdhitsolutions I'd love to better understand why a user may want multiple SecretStore instances in a particular user scope, does having secretmetadata help users to organize their secrets in a way that doesnt require a second instance of the store?
Agree that we can make this more clear in the SecretStore documentation

I might want one store with data that is associated with a module or CI/CD project I'm running and a second store for personal items. I might want one group of data to have a longer unlock time and another set to be more secure. A single store means every secret is treated the same and maybe that isn't what I want.


Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib

DRILL-7882: Fix LGTM Alerts in common folder
#DRILL-7883: Fix LGTM Alerts in contrib folder
Done
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/common/src/main/java/org/apache/drill/common/config/DrillProperties.java?sort=name&dir=ASC&mode=heatmap
DrillProperties.java
line 115
Added synchronized keyword to method
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/common/src/main/java/org/apache/drill/common/HistoricalLog.java?sort=name&dir=ASC&mode=heatmap
HistoricalLog.java
Line 122 & 129
Suppressed b/c they were comments
in format-excel
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java#x4f0d3a45123fb50b:1
excelbatchreader.java
line 280
checked if datacell is null before switch case statement
in format-hdf5
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/HDF5BatchReader.java?sort=name&dir=ASC&mode=heatmap
HDF5BatchReader.java
line 593
Changed {} to %s so that format call works
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5ByteDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5ByteDataWriter.java
line 71
Despite the fact the counter only runs after, and that if write() would run again it would return false due to the if statement beforehand. LGTM still picks up on it. Thus, I have  used a try and catch statement as a way to avoid the alert (although I have no way of testing unless its on the main repo).
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5DoubleDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5DoubleDataWriter.java
line 69
Same as above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5FloatDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5FloatDataWriter.java
line 69
same as above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5IntDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5IntDataWriter.java
line 70
same as above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5LongDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5LongDataWriter.java
line 69
same as above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5SmallIntDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5SmallIntDataWriter.java
line 71
same as above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5TimestampDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5TimeSTampDataWriter
line 48
same as above
in format-img
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-image/src/main/java/org/apache/drill/exec/store/image/GenericMetadataDescriptor.java?sort=name&dir=ASC&mode=heatmap
GenericMetadataDescriptor.java
line 82,83,84
Converted type from Integer to int (didnt seem like there was a need for Integer Class)
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-image/src/main/java/org/apache/drill/exec/store/image/ImageDirectoryProcessor.java?sort=name&dir=ASC&mode=heatmap
ImageDirectoryProcessor.java
line 124
Suppressed, needs to be initialized with an arbitrary value (so keep it with null)
in format-maprdb
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/planner/index/MapRDBStatistics.java?sort=name&dir=ASC&mode=heatmap
line 777
Added if (pattern != null) statement to avoid potential NPE error
line 801
same as above
line 807
if statement but for escape
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/binary/BinaryTableGroupScan.java?sort=name&dir=ASC&mode=heatmap
BinaryTableGroupScan.java
line 190
To avoid int overflow, made numColumns a long variable
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/JsonTableGroupScan.java?sort=name&dir=ASC&mode=heatmap
JsonTableGroupScan
JsonTableGroupScan.java
Line 380
boolean includes scanSpec != null (if spanspec is null then scanspec.getserializedfilter() would also be null)
line 493
suppressed because its comments
line 520
The 5th format call is for the estimated size, but there is no fn that gets/determines the estimated size... For now I put in "Can't determine estimated size"
left it empty
line 527, 528, 541, 542, 632
All are comments, suppressed
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/MaprDBJsonRecordReader.java?sort=name&dir=ASC&mode=heatmap
MaprDBJsonRecordReader.java
Line 431
Changed suppression to suggestion that CodeQL ppl suggested
  document == null || document.asReader() == null ? ...
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBFormatPluginConfig.java?sort=name&dir=ASC&mode=heatmap
MapRDBFormatPluginConfig.java
Line 28
There is no equals function (there is impEquals, but is that the same thing?) but there is an overridden hashcode function. Again, I don't know what it's referring to.
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBGroupScan.java?sort=name&dir=ASC&mode=heatmap
MapRDBGroupScan.java
line 255
Format call had wrong syntax (for format(), use % not {} to take in args)
line 324
It already logs an error if null so there is no point in catching NPE, suppressed
In format-xml
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-xml/src/main/java/org/apache/drill/exec/store/xml/XMLBatchReader.java?sort=name&dir=ASC&mode=heatmap
XMLBatchReader.java
line 94
changed {} to %s
In storage-druid
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidGroupScan.java?sort=name&dir=ASC&mode=heatmap
DruidGroupScan.java
line 201
Added L for long type specification
In storage-hbase
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseUtils.java?sort=name&dir=ASC&mode=heatmap
HBaseUtils.java
line 79
Imported java.utils.Arrays to directly convert filterBytes to a string so that it does not implicitly convert it in the error msg
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java?sort=name&dir=ASC&mode=heatmap
HBasePersistentStore.java
line 201
suppressed b/c its in try catch statement
In storage-kudu
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-kudu/src/main/java/org/apache/drill/exec/store/kudu/KuduGroupScan.java?sort=name&dir=ASC&mode=heatmap
KuduGroupScan.java
line 210
Added L for long type specification
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/udfs/src/main/java/org/apache/drill/exec/udfs/NetworkFunctions.java?sort=name&dir=ASC&mode=heatmap
NetworkFunctions.java
Line 434
Multiplied long with assignment so that there is no implicit conversion
TO-DO (revise) / ASK FOR HELP
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/JsonTableRangePartitionFunction.java?sort=name&dir=ASC&mode=heatmap
JsonTableRangePartitionFunction.java
Line 46
There is an overridden equals function but no hashcode function so I don't know what it's referring to
Suppressed
In storage-kafka
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-kafka/src/main/java/org/apache/drill/exec/store/kafka/KafkaPartitionScanSpec.java?sort=name&dir=ASC&mode=heatmap
KafkaPartitionScanSpec.java
line 25
There is no hashcode fn, if there is no fn, then there's no need for it, suppressed
In udfs
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/udfs/src/main/java/org/apache/drill/exec/udfs/CryptoFunctions.java?sort=name&dir=ASC&mode=heatmap
CryptoFunctions.java
line 288
Alert recommends using AES, but code already uses AES encryption.
Maybe the alternatives are weak? Suppressed for now.
line 339
Same reason as above
In storage-splunk
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-splunk/src/main/java/org/apache/drill/exec/store/splunk/SplunkBatchReader.java?sort=name&dir=ASC&mode=heatmap
SplunkBatchReader.java
line 232
Very unsure. I am not aware of what the contents are so I can't really analyze this. Suppressed for now.
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBTableCache.java?sort=name&dir=ASC&mode=heatmap
MapRDBTableCache.java
Line 73
If table is null, I believe that it should just throw an NPE regardless (especially if its required in maprdb altho im not entirely sure), it's suppressed
perhaps i could use a logger.debug() or something like that
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/streams/StreamsFormatPluginConfig.java?sort=name&dir=ASC&mode=heatmap
StreamsFormatpluginConfig.javas
line 27
Suppressed, hashcode and impequals are both overridden. From what I understand about the alert is trying to find hashcode and equals fn, but because it cant find equals (which is just impequals) it assumes that its not overridden
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/TableFormatPluginConfig.java?sort=name&dir=ASC&mode=heatmap
TableFormatPluginConfig.java
Line 22
Same reason as above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/common/src/main/java/org/apache/drill/common/exceptions/UserException.java?sort=name&dir=ASC&mode=heatmap
UserException.java
Line 481
Suppressed first b/c suggested solution was not valid, format fn needs those 2 parameters
Line 643
It's already in a try statement
alert suppressed
Documentation
N/A
Testing
Due to how LGTM alerts work, if it isnt on the actual project, we can't see if it works.

As a side note: I had not touched Drillbuf.java, or Drillfilesystem.java, but upon making this I found that these changes. I can remove them if they're unnecessary.

Errors in CI involve drillbuf.java, will remove.

@eevanwong
Thank you for the submission.  I see a lot of suppressions here.  Are you going to fix some of these, or just suppress the warnings?

Hey, yeah there are a lot of suppressions. The next step for me is to fully understand all the ??? ones, then determining if I can fix. I just wanted to make a pr now so I didnt have to do it later.

Hey, could I get someones input on this? This is quite a small thing, but still something I want to make sure.

This is in JsonTableGroupScan.java, and essentially the alert is that there were only 4 arg to support the 5 that the format call was requesting for. The thing is, I can't find a fn in indexDesc that deals with finding the estimatedSize. For now, I put in some default string, but I would like to know if I could potentially find estimated size, or I could just delete that portion in the format call.

The most common issues that I suppressed involved null values and potential NPE and override hashcode and equals.  The thing with the null values is that, most times in the codebase, if the value was null it would already be dealt with; however, it still marks it as an alert. With hashcode and equals, there were sometimes a need for only hashcode and not equals, and vice versa.  If one or the other were not present in the file, it would throw an alert. Also, in some cases, the equals function was there, but it was not named equals (it was named impEquals), which LGTM prob did not pick up, and threw the alert.

@eevanwong I will check the questions in your comments tomorrow.

Hey, could I get someones input on this? This is quite a small thing, but still something I want to make sure.

This is in JsonTableGroupScan.java, line 519. The alert is that there were only 4 arg to support the 5 that the format call was requesting for. The thing is, I can't find a fn in indexDesc that deals with finding the estimatedSize. For now, I put in some default string, but I would like to know if I could potentially find estimated size, or I could just delete that portion in the format call.

About all the alarms of the MapR module, you can ignore them directly (Maybe you only need to mark as comments in this PR).

The most common issues that I suppressed involved null values and potential NPE and override hashcode and equals. The thing with the null values is that, most times in the codebase, if the value was null it would already be dealt with (via null guard or logger); however, it still marks it as an alert. With hashcode and equals, there were sometimes a need for only hashcode and not equals, and vice versa. If one or the other were not present in the file, it would throw an alert. Also, in some cases, the equals function was there, but it was not named equals (it was named impEquals), which LGTM prob did not pick up, and threw the alert.

The most common issues that I suppressed involved null values and potential NPE and override hashcode and equals. The thing with the null values is that, most times in the codebase, if the value was null it would already be dealt with (via null guard or logger); however, it still marks it as an alert. With hashcode and equals, there were sometimes a need for only hashcode and not equals, and vice versa. If one or the other were not present in the file, it would throw an alert. Also, in some cases, the equals function was there, but it was not named equals (it was named impEquals), which LGTM prob did not pick up, and threw the alert.

I have noticed your question. Before resolve, I would like to ask you to research the following:

How does LGTM.com work?
Are all alarms meaningful on LGTM.com (Help focus your mind)?
Could you please explain what is LGTM's suggestions for handle the issues (NPE, hashcode)?

According to their website, LGTM takes the codebase, "generates a database to represent the codebase", then uses codeQL to run queries on the database to generate alerts. They use data science to identify the errors (from analyzing a lot of other projects and finding common errors).

It depends. Sometimes, alerts are a false-positive (incorrect); however, pertaining to drill, most of the alerts were meaningful as they were valid issues that needed to be fixed; however, some of them are not. For example,

This already has a guard if the counter is greater than the length of the data, despite that, it still throws the alert, and so I tried to use a try and catch statement.
Also, like I said in my previous comment. Sometimes the value is initialized in the null value as an arbitrary value so that it can be used later. Just declaring it isn't enough as it causes an error.

For hashcode, it recommends overriding both hashCode and equals methods to ensure they're consistent. The thing is, in most cases, there is no hashCode or equals method.

e.g

This results in this hashcode alert

For nulls, it recommends to ensure that the value is not null when its dereferenced.

The thing, the alert is essentially explaining that the value might be null b/c it was initialized as so, but thats because it has to be arbitrary initialized so that it can be called in different statements.

@eevanwong Thanks. I don't like the false-positive (incorrect). It can take a lot of time.

About overriding hashCode and equals. I holding the review. If possible to count the total number of alerts related to this issues?
Nullable. It's related to the heap memory of JVM (Garbage Collection Algorithm)? Ensure the variable is not null for ever?

Yes there are 17 alerts in total caused by this issue.

I'm not sure if the null errors are related, the main issue on the alert was that it would point to an NPE if its value didnt change by the time it got to a statement that would use it.

I suppose I could try ensuring var is not null forever; however, it would just be towards another arbitrary value and it would be odd to do it depending on what type of variable it is (e.g File variable I wouldnt know what value to point it towards).

@eevanwong Thanks for doing this. This job is not easy, because there are many things that can be done or not. Is possible to contact the LGTM.com request a reply about these issues and our thoughts (with the odd recommendations)?

Yeah for sure I'll see what I can do.

Hey so, a bit of an update, for hash code and equals, tuns out you in fact do need both.

Yes, this is a legitimate concern. The contract between hashCode and equals is that equal instances must have the same hashCode. The reverse is not necessary, but high quality hashCode algorithms should avoid this as much as possible.

According to javadoc for the equals method:
Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.

Full javadoc is here: equals and hashCode.

Yes, there is still a few changes I can make before moving onto the uncertain ones.
Yeah I'll work on what I can for now (altho I'm a little busy during the weekend so it'll have to wait)
I agree the overview is a bit messy, I'll reorganize it.

Hi @eevanwong
Could you please rebase with current master?

Sorry, been awhile, I got hard sidetracked by school and whatnot. That being said I think I screwed up the rebase. :/

I rebased successfully, when pushing to my original branch there were conflicts so I attempted to pull but then I had to resolve some merge conflicts.

Yeah...
If you can roll back what you just did, here's how I rebase.
git checkout master
git fetch upstream
git rebase upstream/master
git checkout
git rebase master
There may be an easier way, but that way get's the job done.
-- C

On May 21, 2021, at 5:46 PM, Evan W. @.***> wrote:
Sorry, been awhile, I got hard sidetracked by school and whatnot. That being said I think I screwed up the rebase. :/
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/apache/drill/pull/2191#issuecomment-846278755, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKB7PSNRR2XICHYMCIUSLLTO3ICHANCNFSM4ZVOWFHQ.

Ok, I think it worked

Note:  LGTM is shutting down in December 2022.  See https://github.blog/2022-08-15-the-next-step-for-lgtm-com-github-code-scanning/.


Update build script

This PR tackles #197 by removing the usage of the configuration file and only depending on the term_versions table.
@peterdesmet the output inde.md is available and requries a check. Can I 'deploy' this myself (I presume it gets included in the pelican tdwg pages?) or can you do a check yourself?

First issue: all terms of the dwc: namespace have disappeared. 😅

@tucotuco why do all dwc terms have status new? shouldn't that be recommended?
https://github.com/tdwg/dwc/blob/886b55fbdb50ddfbf24de6af4d64f0953d90849c/vocabulary/term_versions.csv#L17-L19

Yep, 'recommended' instead of 'new'. The 'new' was so I could keep track
during the process of making edits, but I forgot to change it back to
'recommended' afterward.
On Mon, Oct 15, 2018 at 8:39 AM Peter Desmet<EMAIL_ADDRESS>wrote:

@peterdesmet approved this pull request.
Now also included:

Recommended terms have status recommended, not new ping @tucotuco
https://github.com/tucotuco
Examples are extra element for each term

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/tdwg/dwc/pull/206#pullrequestreview-164663807, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAcP67oK7xr1tLCv-wb__66F0flhNY5wks5ulHPhgaJpZM4XbDte
.